feat: allow command to be definted with list#299
Open
NSPC911 wants to merge 9 commits into
Open
Conversation
Reviewer's GuideAllow configuration-driven run commands (right-click actions, editors, openers) to be specified as either strings or string lists, and update command construction, execution, and schema/typing to support this while improving deep-merge error handling. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details
|
| Layer / File(s) | Summary |
|---|---|
Schema and type-stub contract for _Run src/rovr/config/schema.json, src/rovr/classes/config.pyi |
Adds definitions.run (oneOf: string or non-empty string-array) to schema.json and $refs it from all inline run field definitions. Introduces _Run = str | list[str] and _RunOneof1 = list[str] in config.pyi, updating every run TypedDict field across right-click actions, editor file/folder/bulk, and openers to Required["_Run"] or "_Run". |
command() and run_command() refactor src/rovr/functions/utils.py, src/rovr/screens/typed.py |
Rewrites command() to accept str | list[str] | tuple[str] and removes the is_shell parameter; list/tuple inputs append path_str directly, string inputs use shlex.quote. Broadens expand_command() to handle both forms. Adjusts run_command() so shlex.split is applied only when shell is disabled and command is a string; shlex.join is applied when shell is enabled and command is a list. Changes orphan subprocess handling to use subprocess.DEVNULL instead of subprocess.PIPE. Widens ShellExecReturnType.command to str | list[str]. |
Config loading and error handling src/rovr/functions/config.py |
Updates load_config to expand run as a list (per-element os.path.expandvars, editor() fallback on empty) or the existing string path with placeholder-token detection. Changes deep_merge's TypeError handler from exit(1) to raise, with docstring update. |
Editor and action button call sites src/rovr/core/file_list.py, src/rovr/action_buttons/new_item_button.py, src/rovr/action_buttons/rename_item_button.py, src/rovr/core/file_list_right_click_menu.py |
Updates all utils.command(run, path) calls to omit the shell argument. Removes shlex and which imports from file_list.py. Rewrites openers loop in file_selected_handler to derive runner directly from the opener value (string as-is, dict uses opener["run"]), removes executable-existence checking, derives shell from opener.get("shell", True), and inspects proc.returncode for suspend run types to continue on failure or stop on success. Handles run as str | list[str] in file_list_right_click_menu.py when computing disabled state, and corrects orphan gating to check run_type instead of run value. |
Schema reference docs for run oneOf docs/src/content/docs/dev/reference/schema.mdx, AGENTS.md |
Updates all run field rows in the docs to the combining object format with oneOf branches (item 0: string, item 1: array of strings), and expands the table of contents with the nested run oneOf link targets. Adds a note in AGENTS.md documenting that generated schema files must not be edited directly. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title matches the main change: commands can now be defined as lists as well as strings. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
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 help to get the list of available commands.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
run_command, the previous conversion of list commands to strings whenshell=Truehas been removed; passing a list tosubprocesswithshell=Trueis not supported, so you likely still need toshlex.joinlists in that branch to avoid runtime errors when a list-based command is used withshell=True. - In the openers logic in
file_list.py,runnerno longer getstarget_pathappended (it used to be... + [target_path]), so commands that expect the file path argument will now be invoked without it; consider preserving the previous behavior by appending the path when constructingrunner. - The schema docs for
runin the editor/right-click sections appear to reuse anchors and labels from the right-click action definition (e.g.,settings_right_click_items_action_oneOf_i1_run_oneOf_i0) in multiple contexts, which makes the documentation confusing; it would be clearer to generate or adjust distinct anchors/titles per section.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `run_command`, the previous conversion of list commands to strings when `shell=True` has been removed; passing a list to `subprocess` with `shell=True` is not supported, so you likely still need to `shlex.join` lists in that branch to avoid runtime errors when a list-based command is used with `shell=True`.
- In the openers logic in `file_list.py`, `runner` no longer gets `target_path` appended (it used to be `... + [target_path]`), so commands that expect the file path argument will now be invoked without it; consider preserving the previous behavior by appending the path when constructing `runner`.
- The schema docs for `run` in the editor/right-click sections appear to reuse anchors and labels from the right-click action definition (e.g., `settings_right_click_items_action_oneOf_i1_run_oneOf_i0`) in multiple contexts, which makes the documentation confusing; it would be clearer to generate or adjust distinct anchors/titles per section.
## Individual Comments
### Comment 1
<location path="src/rovr/functions/utils.py" line_range="146" />
<code_context>
- if shell and isinstance(command, list):
- from shlex import join
-
- command = join(command)
- elif not shell and isinstance(command, str):
+ if not shell and isinstance(command, str):
</code_context>
<issue_to_address>
**issue (bug_risk):** Changing how list commands are handled when `shell=True` can break subprocess invocation.
Previously, list commands with `shell=True` were normalized to strings via `shlex.join` before invoking `subprocess`; now they’re passed through as lists. Although `subprocess` accepts sequences, doing so with `shell=True` changes how the shell interprets the command and can be platform-dependent, risking regressions for existing callers. If the intent is to prefer argv lists for `shell=False` and strings for `shell=True`, it’d be safer to keep joining lists when `shell=True` and enforce that policy explicitly rather than altering behaviour here.
</issue_to_address>
### Comment 2
<location path="src/rovr/functions/config.py" line_range="130-134" />
<code_context>
values + target if prefix == "prepend" else target + values
)
except TypeError as exc:
+ from rich.traceback import Traceback
+
if locals().get("key") is None and locals().get("value") is None:
</code_context>
<issue_to_address>
**🚨 issue (security):** Printing tracebacks with `show_locals=True` can leak sensitive configuration data.
`Traceback(show_locals=True)` dumps all local variables, including user config, to stdout. Since `deep_merge` operates on user-supplied configuration, this may expose paths, tokens, or credentials when errors occur.
Consider:
- Using `show_locals=False` by default and only enabling `show_locals=True` under an explicit debug flag, or
- Narrowing what’s printed to just the conflicting keys/types instead of all locals.
This keeps error reports useful while limiting the risk of leaking sensitive data.
</issue_to_address>
### Comment 3
<location path="docs/src/content/docs/dev/reference/schema.mdx" line_range="85" />
<code_context>
- [2.4. Property `Rovr Config > settings > editor`](#settings_editor)
- [2.4.1. Property `Rovr Config > settings > editor > file`](#settings_editor_file)
- [2.4.1.1. Property `Rovr Config > settings > editor > file > run`](#settings_editor_file_run)
+ - [2.4.1.1.1. Property `Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 0`](#settings_right_click_items_action_oneOf_i1_run_oneOf_i0)
+ - [2.4.1.1.2. Property `Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 1`](#settings_right_click_items_action_oneOf_i1_run_oneOf_i1)
+ - [2.4.1.1.2.1. Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 1 > item 1 item 0](#autogenerated_heading_5)
</code_context>
<issue_to_address>
**issue:** Editor schema sections reference `right_click` paths and anchors, which is confusing and likely incorrect.
These TOC entries appear to have been copied from the right-click action schema, so the editor section is now labeled and anchored as if it were a right-click configuration. If this is reusing a shared `run` definition, the TOC text and IDs should be renamed to use editor-specific terminology and hierarchy to avoid implying that users are editing right-click actions here.
</issue_to_address>
### Comment 4
<location path="pyproject.toml" line_range="131" />
<code_context>
schema-to-dict.sequence = [
"_docscripts_setup",
{ cmd = "jsonschema-gentypes --json-schema ./src/rovr/config/schema.json --python ./src/rovr/classes/config.pyi" },
+ { cmd = 'python -c "c = (x := open(\"./src/rovr/classes/config.pyi\", \"r\")).read().replace(\"tuple[str]\", \"list[str]\");x.close();(x := open(\"./src/rovr/classes/config.pyi\", \"w\")).write(c)"' },
"format"
]
</code_context>
<issue_to_address>
**suggestion:** Inline `python -c` patch for `config.pyi` is hard to maintain and can be error-prone.
The inline `python -c` that does a raw `.replace("tuple[str]", "list[str]")` on `config.pyi` is brittle: it depends on that exact string only appearing in the intended place, and any change in generator output/formatting could either miss or incorrectly modify content. In-place editing with manual open/close also hurts readability.
Please move this logic into a small, named script (e.g. `tools/fix_config_types.py`) that performs a more targeted transformation, and invoke that script from `schema-to-dict` for a clearer, safer pipeline.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rovr/config/schema.json`:
- Around line 11-19: The run property's array schema currently uses tuple
validation with items defined as an array containing a single string type
object, which only constrains the first element—elements at higher indices pass
validation by default. Convert the items definition from array-form (items as an
array with one object) to object-form (items as a single object) so that all
array elements are consistently validated as strings. Additionally, add a
minItems constraint set to 1 to ensure the array is not empty, since a command
needs at least one item to execute.
- Around line 390-391: In src/rovr/functions/config.py around lines 632-634, the
code assumes raw_run is always a string and directly calls
os.path.expandvars(raw_run), but the schema now allows run to be a list. Modify
the logic to check if raw_run is a list or string first: if it's a string, apply
os.path.expandvars() to it as currently done; if it's a list, apply
os.path.expandvars() to each element in the list. Apply the same type-checking
logic to the token-matching check on line 634 that currently assumes a string.
This fix must be applied to all three editor run configurations (file.run,
folder.run, and bulk_editor.run) that use this same pattern.
In `@src/rovr/core/file_list.py`:
- Around line 428-430: The runner construction for the opener is incorrectly
using list(opener["run"]) which converts a string command like "ls" into a
character array ["l", "s"] instead of properly parsing the command.
Additionally, unlike the editor configuration (line 400), the target_path is not
being appended to the command via utils.command(), and the shell parameter from
the opener config is not being passed to utils.run_command() (unlike line 405).
To fix this, use utils.command(opener["run"], target_path) to properly append
the target path and construct the command, extract the actual command name for
the which() check using shlex.split() to parse the command string correctly, and
pass the shell parameter from opener config to utils.run_command() to respect
the opener's shell configuration.
In `@src/rovr/functions/config.py`:
- Line 143: In the Traceback call on line 143, change show_locals=True to
show_locals=False to prevent exposing sensitive configuration values in crash
output. Alternatively, you can gate the show_locals parameter behind an explicit
debug flag (such as an environment variable or a debug configuration option) so
that detailed local variable output is only shown when explicitly enabled for
debugging purposes.
In `@src/rovr/functions/utils.py`:
- Around line 145-148: The code currently converts string commands to lists when
shell=False, but does not handle the inverse case where shell=True and the
command is passed as a list. When subprocess receives a list with shell=True, it
treats it as a raw filename rather than a shell-executable string. Add an
additional condition to check if shell is True and command is a list, then use
shlex.join() (imported from the shlex module) to normalize the list command into
a properly quoted shell string. This should be implemented alongside the
existing string-to-list conversion logic for shell=False.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c4150b8c-a6c4-462a-b3f5-ea9c931beb7d
📒 Files selected for processing (9)
docs/src/content/docs/dev/reference/schema.mdxpyproject.tomlsrc/rovr/action_buttons/new_item_button.pysrc/rovr/action_buttons/rename_item_button.pysrc/rovr/classes/config.pyisrc/rovr/config/schema.jsonsrc/rovr/core/file_list.pysrc/rovr/functions/config.pysrc/rovr/functions/utils.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Sourcery review
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python version 3.12+ is required
Use double quotes (") for string literals
Use 4 spaces for indentation
Use absolute imports only, no relative imports
All functions must have type annotations following ANN rules
Do not useif TYPE_CHECKINGblocks for imports
UseCamelCasefor class names (e.g.,FileList,Application)
Usesnake_casefor function and variable names (e.g.,get_filtered_dir_names)
UseUPPER_CASEfor constants (e.g.,MAX_ITEMS)
Prefix private functions and variables with underscore (e.g.,_internal_helper)
Use specific exceptions, avoid bareexcept:clauses
Usesuppressfrom contextlib for expected errors
Textual widgets should inherit withinherit_bindings=Falsefor custom keybindings
Use@on(EventType)decorators oron_eventmethods for Textual event handling
Use@workdecorator for async background work in Textual, withthread=Truefor blocking tasks
Usecall_from_thread,call_next,call_later, orcall_after_refreshfor widget modifications in threads
Event handlers in Textual do not need to call their super() method; useprevent_default()to stop event bubbling
Userufffor code formatting, not black
Avoid creating functions in the same file asProcessPoolExecutororProcessto prevent config reload issues
Avoid adding unnecessary comments; aim for self-documenting code instead
Usingsubprocess.run(shell=True)is allowed as users control the config
Files:
src/rovr/action_buttons/rename_item_button.pysrc/rovr/action_buttons/new_item_button.pysrc/rovr/functions/config.pysrc/rovr/functions/utils.pysrc/rovr/core/file_list.py
🧠 Learnings (7)
📚 Learning: 2025-12-11T03:10:54.344Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 167
File: src/rovr/footer/process_container.py:346-346
Timestamp: 2025-12-11T03:10:54.344Z
Learning: In Python/Textual code, when calling checker_wrapper() or any other method that starts workers from within a work(thread=True) decorated method, do not wrap the call in call_from_thread. Starting workers is thread-safe in Textual and the work decorator handles threading. Only UI updates (e.g., DOM modifications) from worker threads require call_from_thread. Apply this guideline to Python files across the codebase where worker-start calls occur; do not wrap such calls with call_from_thread.
Applied to files:
src/rovr/action_buttons/rename_item_button.pysrc/rovr/action_buttons/new_item_button.pysrc/rovr/functions/config.pysrc/rovr/functions/utils.pysrc/rovr/core/file_list.py
📚 Learning: 2026-01-24T02:10:57.940Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 203
File: src/rovr/first_launch.py:273-295
Timestamp: 2026-01-24T02:10:57.940Z
Learning: In the rovr codebase (Python/Textual), App.notify() is thread-safe and may be called directly from worker threads decorated with work(thread=True) without wrapping in call_from_thread(). Only DOM modifications require call_from_thread from worker threads. Apply this guideline to Python files across the codebase (not just a single file) and avoid unnecessary call_from_thread() wrappers when invoking App.notify() from worker threads.
Applied to files:
src/rovr/action_buttons/rename_item_button.pysrc/rovr/action_buttons/new_item_button.pysrc/rovr/functions/config.pysrc/rovr/functions/utils.pysrc/rovr/core/file_list.py
📚 Learning: 2026-01-30T13:54:15.110Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 207
File: src/rovr/first_launch.py:96-107
Timestamp: 2026-01-30T13:54:15.110Z
Learning: In Python projects using the Textual framework, async workers decorated with work are automatically cancelled when the parent screen/widget is dismissed or unmounted. Do not add manual cancellation in cleanup/dismiss handlers for these workers; rely on Textual to cancel them. Verify this behavior against the project’s Textual version before relying on it.
Applied to files:
src/rovr/action_buttons/rename_item_button.pysrc/rovr/action_buttons/new_item_button.pysrc/rovr/functions/config.pysrc/rovr/functions/utils.pysrc/rovr/core/file_list.py
📚 Learning: 2026-02-27T09:15:49.243Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 229
File: src/rovr/functions/system_clipboard.py:143-143
Timestamp: 2026-02-27T09:15:49.243Z
Learning: In Python files that display error messages in toast/notification UI (e.g., ClipboardError and similar exceptions), escape unescaped square brackets in the message so the toast system does not interpret them as markup. Specifically render the content with double backslashes before brackets, e.g. "\\[{type(exc).__name__}]" in the toast description. This guideline applies to all Python files that format and show exception details in toasts across the codebase.
Applied to files:
src/rovr/action_buttons/rename_item_button.pysrc/rovr/action_buttons/new_item_button.pysrc/rovr/functions/config.pysrc/rovr/functions/utils.pysrc/rovr/core/file_list.py
📚 Learning: 2025-12-11T03:11:24.387Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 167
File: src/rovr/screens/fd_search.py:205-209
Timestamp: 2025-12-11T03:11:24.387Z
Learning: In src/rovr/screens/fd_search.py, FileSearch.FILTER_TYPES should be implemented as a class attribute (not an instance attribute) to persist filter state across all modal instances within the same session. This enables remember/restore of filter settings when the modal is dismissed and reopened. Ensure this behavior is intentional, document the session lifecycle implications, and include tests verifying persistence across modal reopenings, as well as proper reset or isolation when a new session starts. If applicable, consider thread-safety and potential memory growth due to long-lived state.
Applied to files:
src/rovr/action_buttons/rename_item_button.pysrc/rovr/action_buttons/new_item_button.pysrc/rovr/functions/config.pysrc/rovr/functions/utils.pysrc/rovr/core/file_list.py
📚 Learning: 2025-12-11T03:11:27.833Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 167
File: src/rovr/screens/fd_search.py:205-209
Timestamp: 2025-12-11T03:11:27.833Z
Learning: In src/rovr/screens/fd_search.py, document that the FileSearch class uses a class-level FILTER_TYPES dict to persist filter settings across multiple modal invocations within a single application session. Ensure this pattern is intentional by clarifying the lifecycle of the shared state (lifetime of the process/session) and the implications of mutable shared state (thread-safety, potential memory retention). Consider alternatives (per-instance state, dedicated session storage, explicit reset method) and add tests to verify persistence, correctness after modal reopenings, and safe reset behavior. Also mention any framework or runtime constraints that affect shared-class state.
Applied to files:
src/rovr/action_buttons/rename_item_button.pysrc/rovr/action_buttons/new_item_button.pysrc/rovr/functions/config.pysrc/rovr/functions/utils.pysrc/rovr/core/file_list.py
📚 Learning: 2026-01-03T14:17:18.132Z
Learnt from: lazysegtree
Repo: NSPC911/rovr PR: 184
File: src/rovr/core/preview_container.py:280-283
Timestamp: 2026-01-03T14:17:18.132Z
Learning: In src/rovr/core/preview_container.py, mypy may not recognize that pdfinfo_from_path's poppler_path parameter accepts None even though the library signature is Optional[Union[str, pathlib.PurePath]]. If you encounter a type error, explicitly cast the argument to Optional[Union[str, pathlib.PurePath]] before passing it (e.g., from typing import cast; pdfinfo_from_path(path, poppler_path=cast(Optional[Union[str, pathlib.PurePath]], None)) or cast the argument to the expected type). This documents the typing limitation and provides a repeatable workaround for similar cases.
Applied to files:
src/rovr/action_buttons/rename_item_button.pysrc/rovr/action_buttons/new_item_button.pysrc/rovr/functions/config.pysrc/rovr/functions/utils.pysrc/rovr/core/file_list.py
🔇 Additional comments (6)
pyproject.toml (1)
131-131: LGTM!docs/src/content/docs/dev/reference/schema.mdx (1)
52-54: LGTM!Also applies to: 85-101, 999-1050, 1442-1495, 1528-1581, 1614-1667, 1786-1799
src/rovr/classes/config.pyi (1)
359-361: LGTM!Also applies to: 1460-1461, 1480-1482, 1506-1508, 1537-1540, 1764-1765
src/rovr/core/file_list.py (1)
400-400: LGTM!Also applies to: 942-942
src/rovr/action_buttons/new_item_button.py (1)
149-149: LGTM!src/rovr/action_buttons/rename_item_button.py (1)
124-124: LGTM!
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rovr/functions/config.py`:
- Around line 632-643: The list branch (when raw_run is a list) needs to apply
the same unresolved-editor-token fallback logic as the string branch. After
expanding variables in the list with os.path.expandvars, add a check to see if
any of the expanded parts still contain unresolved editor tokens like "$EDITOR",
"${EDITOR}", or "%EDITOR%". If such tokens are found in the expanded_run list,
set expanded_run to [editor()] to provide a proper fallback, matching the
behavior of the string branch which already performs this check.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3514ec78-6ed7-4327-926f-075253e70402
📒 Files selected for processing (4)
docs/src/content/docs/dev/reference/schema.mdxsrc/rovr/classes/config.pyisrc/rovr/config/schema.jsonsrc/rovr/functions/config.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/rovr/config/schema.json
- docs/src/content/docs/dev/reference/schema.mdx
- src/rovr/classes/config.pyi
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python version 3.12+ is required
Use double quotes (") for string literals
Use 4 spaces for indentation
Use absolute imports only, no relative imports
All functions must have type annotations following ANN rules
Do not useif TYPE_CHECKINGblocks for imports
UseCamelCasefor class names (e.g.,FileList,Application)
Usesnake_casefor function and variable names (e.g.,get_filtered_dir_names)
UseUPPER_CASEfor constants (e.g.,MAX_ITEMS)
Prefix private functions and variables with underscore (e.g.,_internal_helper)
Use specific exceptions, avoid bareexcept:clauses
Usesuppressfrom contextlib for expected errors
Textual widgets should inherit withinherit_bindings=Falsefor custom keybindings
Use@on(EventType)decorators oron_eventmethods for Textual event handling
Use@workdecorator for async background work in Textual, withthread=Truefor blocking tasks
Usecall_from_thread,call_next,call_later, orcall_after_refreshfor widget modifications in threads
Event handlers in Textual do not need to call their super() method; useprevent_default()to stop event bubbling
Userufffor code formatting, not black
Avoid creating functions in the same file asProcessPoolExecutororProcessto prevent config reload issues
Avoid adding unnecessary comments; aim for self-documenting code instead
Usingsubprocess.run(shell=True)is allowed as users control the config
Files:
src/rovr/functions/config.py
🧠 Learnings (7)
📚 Learning: 2025-12-11T03:10:54.344Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 167
File: src/rovr/footer/process_container.py:346-346
Timestamp: 2025-12-11T03:10:54.344Z
Learning: In Python/Textual code, when calling checker_wrapper() or any other method that starts workers from within a work(thread=True) decorated method, do not wrap the call in call_from_thread. Starting workers is thread-safe in Textual and the work decorator handles threading. Only UI updates (e.g., DOM modifications) from worker threads require call_from_thread. Apply this guideline to Python files across the codebase where worker-start calls occur; do not wrap such calls with call_from_thread.
Applied to files:
src/rovr/functions/config.py
📚 Learning: 2026-01-24T02:10:57.940Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 203
File: src/rovr/first_launch.py:273-295
Timestamp: 2026-01-24T02:10:57.940Z
Learning: In the rovr codebase (Python/Textual), App.notify() is thread-safe and may be called directly from worker threads decorated with work(thread=True) without wrapping in call_from_thread(). Only DOM modifications require call_from_thread from worker threads. Apply this guideline to Python files across the codebase (not just a single file) and avoid unnecessary call_from_thread() wrappers when invoking App.notify() from worker threads.
Applied to files:
src/rovr/functions/config.py
📚 Learning: 2026-01-30T13:54:15.110Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 207
File: src/rovr/first_launch.py:96-107
Timestamp: 2026-01-30T13:54:15.110Z
Learning: In Python projects using the Textual framework, async workers decorated with work are automatically cancelled when the parent screen/widget is dismissed or unmounted. Do not add manual cancellation in cleanup/dismiss handlers for these workers; rely on Textual to cancel them. Verify this behavior against the project’s Textual version before relying on it.
Applied to files:
src/rovr/functions/config.py
📚 Learning: 2026-02-27T09:15:49.243Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 229
File: src/rovr/functions/system_clipboard.py:143-143
Timestamp: 2026-02-27T09:15:49.243Z
Learning: In Python files that display error messages in toast/notification UI (e.g., ClipboardError and similar exceptions), escape unescaped square brackets in the message so the toast system does not interpret them as markup. Specifically render the content with double backslashes before brackets, e.g. "\\[{type(exc).__name__}]" in the toast description. This guideline applies to all Python files that format and show exception details in toasts across the codebase.
Applied to files:
src/rovr/functions/config.py
📚 Learning: 2025-12-11T03:11:24.387Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 167
File: src/rovr/screens/fd_search.py:205-209
Timestamp: 2025-12-11T03:11:24.387Z
Learning: In src/rovr/screens/fd_search.py, FileSearch.FILTER_TYPES should be implemented as a class attribute (not an instance attribute) to persist filter state across all modal instances within the same session. This enables remember/restore of filter settings when the modal is dismissed and reopened. Ensure this behavior is intentional, document the session lifecycle implications, and include tests verifying persistence across modal reopenings, as well as proper reset or isolation when a new session starts. If applicable, consider thread-safety and potential memory growth due to long-lived state.
Applied to files:
src/rovr/functions/config.py
📚 Learning: 2025-12-11T03:11:27.833Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 167
File: src/rovr/screens/fd_search.py:205-209
Timestamp: 2025-12-11T03:11:27.833Z
Learning: In src/rovr/screens/fd_search.py, document that the FileSearch class uses a class-level FILTER_TYPES dict to persist filter settings across multiple modal invocations within a single application session. Ensure this pattern is intentional by clarifying the lifecycle of the shared state (lifetime of the process/session) and the implications of mutable shared state (thread-safety, potential memory retention). Consider alternatives (per-instance state, dedicated session storage, explicit reset method) and add tests to verify persistence, correctness after modal reopenings, and safe reset behavior. Also mention any framework or runtime constraints that affect shared-class state.
Applied to files:
src/rovr/functions/config.py
📚 Learning: 2026-01-03T14:17:18.132Z
Learnt from: lazysegtree
Repo: NSPC911/rovr PR: 184
File: src/rovr/core/preview_container.py:280-283
Timestamp: 2026-01-03T14:17:18.132Z
Learning: In src/rovr/core/preview_container.py, mypy may not recognize that pdfinfo_from_path's poppler_path parameter accepts None even though the library signature is Optional[Union[str, pathlib.PurePath]]. If you encounter a type error, explicitly cast the argument to Optional[Union[str, pathlib.PurePath]] before passing it (e.g., from typing import cast; pdfinfo_from_path(path, poppler_path=cast(Optional[Union[str, pathlib.PurePath]], None)) or cast the argument to the expected type). This documents the typing limitation and provides a repeatable workaround for similar cases.
Applied to files:
src/rovr/functions/config.py
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/rovr/core/file_list.py (1)
427-437:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winOpener execution no longer includes the selected path and ignores
shellconfig.At Line 427 you now pass the opener command straight through, so
target_pathis not appended. At Line 433 you callrun_command(...)withoutshell=..., so opener-levelshell: falseis ignored. Together, this breaks opener behaviour and config intent.Proposed fix
- runner = opener if isinstance(opener, str) else opener["run"] + raw_runner = opener if isinstance(opener, str) else opener["run"] + runner = utils.command(raw_runner, target_path) run_type = ( ("orphan" if opener.get("orphan", True) else "suspend") if isinstance(opener, dict) else "orphan" ) - proc = utils.run_command(self.app, runner, run_type) + shell = opener.get("shell", True) if isinstance(opener, dict) else True + proc = utils.run_command(self.app, runner, run_type, shell=shell) if run_type == "suspend" and proc.returncode != 0: continue opened = True break🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rovr/core/file_list.py` around lines 427 - 437, The opener execution in the section starting with the `runner` variable assignment is missing two important pieces: the target path is not being appended to the runner command, and the shell configuration from the opener is not being passed to the run_command call. Fix this by appending target_path to the runner command (handling both string and dict opener types appropriately), and extract the shell configuration from the opener dict (if it exists and is a dict) and pass it as the shell parameter to the utils.run_command call on line 433 to preserve the opener-level shell configuration intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/src/content/docs/dev/reference/schema.mdx`:
- Around line 1442-1495: The subsections defining the run property under editor
file contain duplicate anchor names
(settings_right_click_items_action_oneOf_i1_run_oneOf_i0 and
settings_right_click_items_action_oneOf_i1_run_oneOf_i1) that conflict with
existing anchors in the right-click action section, and their property paths
incorrectly reference right-click items instead of editor file settings. Since
`#/definitions/run` is a shared schema definition, replace the entire subsection
detailing the run property's oneOf options with a "Same definition as" reference
pattern (following the model used in the openers section), which will eliminate
duplicate anchors and correctly represent the shared schema contract.
- Around line 1614-1667: The anchor names and property paths in the subsections
describing the bulk_editor run property (starting with the section for
settings_editor_bulk_editor_run and continuing through the oneOf items) are
incorrect and duplicate the right_click context rather than reflecting the
bulk_editor context. Replace the detailed subsection documentation (lines
1630-1666) that enumerate the oneOf options with a reference pattern that points
to the shared run definition, similar to how other shared schema properties are
documented, rather than repeating the full schema structure with incorrect
anchor names and property paths.
- Around line 1528-1581: The subsection documentation for the `run` property
under `editor > folder` contains duplicate anchor names and incorrect property
path references that point to `right_click` instead of the correct `editor >
folder` path. Rename the duplicate anchors
`settings_right_click_items_action_oneOf_i1_run_oneOf_i0` and
`settings_right_click_items_action_oneOf_i1_run_oneOf_i1` to use the correct
naming pattern reflecting the editor folder property path (such as
`settings_editor_folder_run_oneOf_i0` and
`settings_editor_folder_run_oneOf_i1`), and update all section titles in lines
1549-1580 to correctly reference `Rovr Config > settings > editor > folder >
run` instead of the incorrect right_click property path references.
In `@src/rovr/config/schema.json`:
- Line 44: The schema now allows run to be a list of strings, but the
right-click execution path still expects a single string value. In the
right-click runtime handler where event.option.action["run"] is passed to
expand_command, update the code to handle both string and list types. Either
normalize the list form to a string before calling expand_command (by joining
list elements), or modify the expand_command function signature to accept str |
list[str] and handle both cases internally. This will ensure the runtime
behavior aligns with the updated schema definition.
In `@src/rovr/functions/config.py`:
- Around line 632-646: The expanded_run values are not being persisted back to
config_dict properly. For list inputs (when isinstance(raw_run, list)), the
expanded_run is computed but never written back to config_dict. For string
inputs (the else branch), the expanded_run is only written back to
config_dict["settings"]["editor"][key]["run"] in the failure branch, not after
successful expansion. Fix this by ensuring that
config_dict["settings"]["editor"][key]["run"] is always assigned the
expanded_run value immediately after expanding it, regardless of whether the
expansion was successful or if it's a list or string input, so that environment
variables like $EDITOR are properly resolved and persisted in the config.
---
Duplicate comments:
In `@src/rovr/core/file_list.py`:
- Around line 427-437: The opener execution in the section starting with the
`runner` variable assignment is missing two important pieces: the target path is
not being appended to the runner command, and the shell configuration from the
opener is not being passed to the run_command call. Fix this by appending
target_path to the runner command (handling both string and dict opener types
appropriately), and extract the shell configuration from the opener dict (if it
exists and is a dict) and pass it as the shell parameter to the
utils.run_command call on line 433 to preserve the opener-level shell
configuration intent.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1051c6ed-6276-4dac-aa75-76f3154e1b9a
📒 Files selected for processing (8)
docs/src/content/docs/dev/reference/schema.mdxsrc/rovr/action_buttons/new_item_button.pysrc/rovr/action_buttons/rename_item_button.pysrc/rovr/classes/config.pyisrc/rovr/config/schema.jsonsrc/rovr/core/file_list.pysrc/rovr/functions/config.pysrc/rovr/functions/utils.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python version 3.12+ is required
Use double quotes (") for string literals
Use 4 spaces for indentation
Use absolute imports only, no relative imports
All functions must have type annotations following ANN rules
Do not useif TYPE_CHECKINGblocks for imports
UseCamelCasefor class names (e.g.,FileList,Application)
Usesnake_casefor function and variable names (e.g.,get_filtered_dir_names)
UseUPPER_CASEfor constants (e.g.,MAX_ITEMS)
Prefix private functions and variables with underscore (e.g.,_internal_helper)
Use specific exceptions, avoid bareexcept:clauses
Usesuppressfrom contextlib for expected errors
Textual widgets should inherit withinherit_bindings=Falsefor custom keybindings
Use@on(EventType)decorators oron_eventmethods for Textual event handling
Use@workdecorator for async background work in Textual, withthread=Truefor blocking tasks
Usecall_from_thread,call_next,call_later, orcall_after_refreshfor widget modifications in threads
Event handlers in Textual do not need to call their super() method; useprevent_default()to stop event bubbling
Userufffor code formatting, not black
Avoid creating functions in the same file asProcessPoolExecutororProcessto prevent config reload issues
Avoid adding unnecessary comments; aim for self-documenting code instead
Usingsubprocess.run(shell=True)is allowed as users control the config
Files:
src/rovr/action_buttons/new_item_button.pysrc/rovr/action_buttons/rename_item_button.pysrc/rovr/functions/config.pysrc/rovr/functions/utils.pysrc/rovr/core/file_list.py
🧠 Learnings (7)
📚 Learning: 2025-12-11T03:10:54.344Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 167
File: src/rovr/footer/process_container.py:346-346
Timestamp: 2025-12-11T03:10:54.344Z
Learning: In Python/Textual code, when calling checker_wrapper() or any other method that starts workers from within a work(thread=True) decorated method, do not wrap the call in call_from_thread. Starting workers is thread-safe in Textual and the work decorator handles threading. Only UI updates (e.g., DOM modifications) from worker threads require call_from_thread. Apply this guideline to Python files across the codebase where worker-start calls occur; do not wrap such calls with call_from_thread.
Applied to files:
src/rovr/action_buttons/new_item_button.pysrc/rovr/action_buttons/rename_item_button.pysrc/rovr/functions/config.pysrc/rovr/functions/utils.pysrc/rovr/core/file_list.py
📚 Learning: 2026-01-24T02:10:57.940Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 203
File: src/rovr/first_launch.py:273-295
Timestamp: 2026-01-24T02:10:57.940Z
Learning: In the rovr codebase (Python/Textual), App.notify() is thread-safe and may be called directly from worker threads decorated with work(thread=True) without wrapping in call_from_thread(). Only DOM modifications require call_from_thread from worker threads. Apply this guideline to Python files across the codebase (not just a single file) and avoid unnecessary call_from_thread() wrappers when invoking App.notify() from worker threads.
Applied to files:
src/rovr/action_buttons/new_item_button.pysrc/rovr/action_buttons/rename_item_button.pysrc/rovr/functions/config.pysrc/rovr/functions/utils.pysrc/rovr/core/file_list.py
📚 Learning: 2026-01-30T13:54:15.110Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 207
File: src/rovr/first_launch.py:96-107
Timestamp: 2026-01-30T13:54:15.110Z
Learning: In Python projects using the Textual framework, async workers decorated with work are automatically cancelled when the parent screen/widget is dismissed or unmounted. Do not add manual cancellation in cleanup/dismiss handlers for these workers; rely on Textual to cancel them. Verify this behavior against the project’s Textual version before relying on it.
Applied to files:
src/rovr/action_buttons/new_item_button.pysrc/rovr/action_buttons/rename_item_button.pysrc/rovr/functions/config.pysrc/rovr/functions/utils.pysrc/rovr/core/file_list.py
📚 Learning: 2026-02-27T09:15:49.243Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 229
File: src/rovr/functions/system_clipboard.py:143-143
Timestamp: 2026-02-27T09:15:49.243Z
Learning: In Python files that display error messages in toast/notification UI (e.g., ClipboardError and similar exceptions), escape unescaped square brackets in the message so the toast system does not interpret them as markup. Specifically render the content with double backslashes before brackets, e.g. "\\[{type(exc).__name__}]" in the toast description. This guideline applies to all Python files that format and show exception details in toasts across the codebase.
Applied to files:
src/rovr/action_buttons/new_item_button.pysrc/rovr/action_buttons/rename_item_button.pysrc/rovr/functions/config.pysrc/rovr/functions/utils.pysrc/rovr/core/file_list.py
📚 Learning: 2025-12-11T03:11:24.387Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 167
File: src/rovr/screens/fd_search.py:205-209
Timestamp: 2025-12-11T03:11:24.387Z
Learning: In src/rovr/screens/fd_search.py, FileSearch.FILTER_TYPES should be implemented as a class attribute (not an instance attribute) to persist filter state across all modal instances within the same session. This enables remember/restore of filter settings when the modal is dismissed and reopened. Ensure this behavior is intentional, document the session lifecycle implications, and include tests verifying persistence across modal reopenings, as well as proper reset or isolation when a new session starts. If applicable, consider thread-safety and potential memory growth due to long-lived state.
Applied to files:
src/rovr/action_buttons/new_item_button.pysrc/rovr/action_buttons/rename_item_button.pysrc/rovr/functions/config.pysrc/rovr/functions/utils.pysrc/rovr/core/file_list.py
📚 Learning: 2025-12-11T03:11:27.833Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 167
File: src/rovr/screens/fd_search.py:205-209
Timestamp: 2025-12-11T03:11:27.833Z
Learning: In src/rovr/screens/fd_search.py, document that the FileSearch class uses a class-level FILTER_TYPES dict to persist filter settings across multiple modal invocations within a single application session. Ensure this pattern is intentional by clarifying the lifecycle of the shared state (lifetime of the process/session) and the implications of mutable shared state (thread-safety, potential memory retention). Consider alternatives (per-instance state, dedicated session storage, explicit reset method) and add tests to verify persistence, correctness after modal reopenings, and safe reset behavior. Also mention any framework or runtime constraints that affect shared-class state.
Applied to files:
src/rovr/action_buttons/new_item_button.pysrc/rovr/action_buttons/rename_item_button.pysrc/rovr/functions/config.pysrc/rovr/functions/utils.pysrc/rovr/core/file_list.py
📚 Learning: 2026-01-03T14:17:18.132Z
Learnt from: lazysegtree
Repo: NSPC911/rovr PR: 184
File: src/rovr/core/preview_container.py:280-283
Timestamp: 2026-01-03T14:17:18.132Z
Learning: In src/rovr/core/preview_container.py, mypy may not recognize that pdfinfo_from_path's poppler_path parameter accepts None even though the library signature is Optional[Union[str, pathlib.PurePath]]. If you encounter a type error, explicitly cast the argument to Optional[Union[str, pathlib.PurePath]] before passing it (e.g., from typing import cast; pdfinfo_from_path(path, poppler_path=cast(Optional[Union[str, pathlib.PurePath]], None)) or cast the argument to the expected type). This documents the typing limitation and provides a repeatable workaround for similar cases.
Applied to files:
src/rovr/action_buttons/new_item_button.pysrc/rovr/action_buttons/rename_item_button.pysrc/rovr/functions/config.pysrc/rovr/functions/utils.pysrc/rovr/core/file_list.py
🔇 Additional comments (8)
docs/src/content/docs/dev/reference/schema.mdx (4)
52-54: ⚡ Quick winVerify these ToC links won't break when the duplicate anchor issue is resolved.
These ToC entries link to
#settings_right_click_items_action_oneOf_i1_run_oneOf_i0and#settings_right_click_items_action_oneOf_i1_run_oneOf_i1, which are correct for the right-click action section. However, the same anchor names are reused in the editor sections below (lines 1463, 1470, etc.), which would cause navigation issues. Once the duplicate anchor problem is fixed, you'll need to ensure these ToC links still resolve to the correct location (presumably the right-click action section, since that's where the shared definition is first introduced).Could you verify the intended navigation structure? If each section should have unique anchors, the ToC will need updating as well.
85-101: ⚡ Quick winToC structure reuses right-click anchors for editor sections.
The ToC entries for editor file, folder, and bulk_editor all reference the same anchor IDs from the right-click action section (
#settings_right_click_items_action_oneOf_i1_run_oneOf_i0and#settings_right_click_items_action_oneOf_i1_run_oneOf_i1). If the intent is to document a shared definition, this might be acceptable, but it creates confusion when users navigate from the editor ToC expecting to see editor-specific documentation. Consider whether each section should have its own anchors (e.g.,#settings_editor_file_run_oneOf_i0) or whether the ToC should explicitly indicate these link to the shared definition.Can you clarify the intended ToC structure for shared definitions?
999-1050: LGTM!
1786-1799: Excellent use of "Same definition as" to avoid duplication.The openers section correctly references the shared
rundefinition without duplicating the oneOf structure. This approach avoids the duplicate anchor and incorrect property path issues present in the editor sections, and accurately represents the fact that#/definitions/runis a shared schema definition. The other sections should follow this pattern.src/rovr/classes/config.pyi (1)
359-361: LGTM!Also applies to: 1460-1461, 1480-1482, 1506-1508, 1537-1540, 1763-1768
src/rovr/functions/utils.py (1)
145-152: LGTM!Also applies to: 162-163, 172-173, 269-279
src/rovr/action_buttons/new_item_button.py (1)
149-149: LGTM!src/rovr/action_buttons/rename_item_button.py (1)
124-124: LGTM!
Comment on lines
+1442
to
+1495
| | [run](#settings_editor_file_run) | object | Editor to use when opening a file | | ||
| | [orphan](#settings_editor_file_orphan) | boolean | Whether to open the editor as an orphan process. | | ||
| | [shell](#settings_editor_file_shell) | boolean | Whether the command to run is a shell command that needs to be run in a shell (required if you want to do complicated things like piping in your editor command) | | ||
|
|
||
| ##### <a name="settings_editor_file_run"></a>2.4.1.1. Property `Rovr Config > settings > editor > file > run` | ||
|
|
||
| | | | | ||
| | ------------ | ----------- | | ||
| | **Type** | `string` | | ||
| | **Required** | Yes | | ||
| | **Default** | `"$EDITOR"` | | ||
| | | | | ||
| | ------------------------- | ----------------- | | ||
| | **Type** | `combining` | | ||
| | **Required** | Yes | | ||
| | **Additional properties** | Any type allowed | | ||
| | **Default** | `"$EDITOR"` | | ||
| | **Defined in** | #/definitions/run | | ||
|
|
||
| **Description:** Editor to use when opening a file | ||
|
|
||
| | One of(Option) | | ||
| | ------------------------------------------------------------------ | | ||
| | [item 0](#settings_right_click_items_action_oneOf_i1_run_oneOf_i0) | | ||
| | [item 1](#settings_right_click_items_action_oneOf_i1_run_oneOf_i1) | | ||
|
|
||
| ###### <a name="settings_right_click_items_action_oneOf_i1_run_oneOf_i0"></a>2.4.1.1.1. Property `Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 0` | ||
|
|
||
| | | | | ||
| | ------------ | -------- | | ||
| | **Type** | `string` | | ||
| | **Required** | No | | ||
|
|
||
| ###### <a name="settings_right_click_items_action_oneOf_i1_run_oneOf_i1"></a>2.4.1.1.2. Property `Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 1` | ||
|
|
||
| | | | | ||
| | ------------ | ----------------- | | ||
| | **Type** | `array of string` | | ||
| | **Required** | No | | ||
|
|
||
| | | Array restrictions | | ||
| | -------------------- | ------------------ | | ||
| | **Min items** | 1 | | ||
| | **Max items** | N/A | | ||
| | **Items unicity** | False | | ||
| | **Additional items** | False | | ||
| | **Tuple validation** | See below | | ||
|
|
||
| | Each item of this array must be | Description | | ||
| | ------------------------------------------------------------------------------ | ----------- | | ||
| | [item 1 items](#settings_right_click_items_action_oneOf_i1_run_oneOf_i1_items) | | | ||
|
|
||
| ###### <a name="settings_right_click_items_action_oneOf_i1_run_oneOf_i1_items"></a>2.4.1.1.2.1. Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 1 > item 1 items | ||
|
|
||
| | | | | ||
| | ------------ | -------- | | ||
| | **Type** | `string` | | ||
| | **Required** | No | | ||
|
|
Contributor
There was a problem hiding this comment.
Duplicate anchor names and incorrect property paths break navigation.
The subsections at lines 1463-1494 define anchors settings_right_click_items_action_oneOf_i1_run_oneOf_i0 and settings_right_click_items_action_oneOf_i1_run_oneOf_i1, which duplicate the anchors already defined in the right-click action section (lines 1019-1050). HTML anchor IDs must be unique within a document, so this would cause navigation failures or unpredictable behaviour.
Additionally, the section titles at lines 1463 and 1470 read "Property Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 0", but these sections appear under the editor file documentation, not right-click actions. The paths should be "Property Rovr Config > settings > editor > file > run > oneOf > item 0" or similar.
Since #/definitions/run is a shared schema definition (confirmed in context snippet 1), the correct approach is to use "Same definition as" with a link to the canonical definition, as demonstrated in the openers section at line 1799. This avoids duplication and correctly represents the shared schema contract.
📚 Recommended fix to reference the shared definition
Replace lines 1458-1494 with a "Same definition as" reference:
-| One of(Option) |
-| ------------------------------------------------------------------ |
-| [item 0](`#settings_right_click_items_action_oneOf_i1_run_oneOf_i0`) |
-| [item 1](`#settings_right_click_items_action_oneOf_i1_run_oneOf_i1`) |
-
-###### <a name="settings_right_click_items_action_oneOf_i1_run_oneOf_i0"></a>2.4.1.1.1. Property `Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 0`
-
-| | |
-| ------------ | -------- |
-| **Type** | `string` |
-| **Required** | No |
-
-###### <a name="settings_right_click_items_action_oneOf_i1_run_oneOf_i1"></a>2.4.1.1.2. Property `Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 1`
-
-| | |
-| ------------ | ----------------- |
-| **Type** | `array of string` |
-| **Required** | No |
-
-| | Array restrictions |
-| -------------------- | ------------------ |
-| **Min items** | 1 |
-| **Max items** | N/A |
-| **Items unicity** | False |
-| **Additional items** | False |
-| **Tuple validation** | See below |
-
-| Each item of this array must be | Description |
-| ------------------------------------------------------------------------------ | ----------- |
-| [item 1 items](`#settings_right_click_items_action_oneOf_i1_run_oneOf_i1_items`) | |
-
-###### <a name="settings_right_click_items_action_oneOf_i1_run_oneOf_i1_items"></a>2.4.1.1.2.1. Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 1 > item 1 items
-
-| | |
-| ------------ | -------- |
-| **Type** | `string` |
-| **Required** | No |
+| **Same definition as** | [run](`#settings_right_click_items_action_oneOf_i1_run`) |
This matches the pattern used for openers at line 1799 and correctly represents the shared #/definitions/run schema.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/src/content/docs/dev/reference/schema.mdx` around lines 1442 - 1495, The
subsections defining the run property under editor file contain duplicate anchor
names (settings_right_click_items_action_oneOf_i1_run_oneOf_i0 and
settings_right_click_items_action_oneOf_i1_run_oneOf_i1) that conflict with
existing anchors in the right-click action section, and their property paths
incorrectly reference right-click items instead of editor file settings. Since
`#/definitions/run` is a shared schema definition, replace the entire subsection
detailing the run property's oneOf options with a "Same definition as" reference
pattern (following the model used in the openers section), which will eliminate
duplicate anchors and correctly represent the shared schema contract.
Comment on lines
+1528
to
+1581
| | [run](#settings_editor_folder_run) | object | Editor to use when opening a folder | | ||
| | [orphan](#settings_editor_folder_orphan) | boolean | Whether to open the editor as an orphan process. | | ||
| | [shell](#settings_editor_folder_shell) | boolean | Whether the command to run is a shell command that needs to be run in a shell (required if you want to do complicated things like piping in your editor command) | | ||
|
|
||
| ##### <a name="settings_editor_folder_run"></a>2.4.2.1. Property `Rovr Config > settings > editor > folder > run` | ||
|
|
||
| | | | | ||
| | ------------ | ----------- | | ||
| | **Type** | `string` | | ||
| | **Required** | Yes | | ||
| | **Default** | `"$EDITOR"` | | ||
| | | | | ||
| | ------------------------- | ----------------- | | ||
| | **Type** | `combining` | | ||
| | **Required** | Yes | | ||
| | **Additional properties** | Any type allowed | | ||
| | **Default** | `"$EDITOR"` | | ||
| | **Defined in** | #/definitions/run | | ||
|
|
||
| **Description:** Editor to use when opening a folder | ||
|
|
||
| | One of(Option) | | ||
| | ------------------------------------------------------------------ | | ||
| | [item 0](#settings_right_click_items_action_oneOf_i1_run_oneOf_i0) | | ||
| | [item 1](#settings_right_click_items_action_oneOf_i1_run_oneOf_i1) | | ||
|
|
||
| ###### <a name="settings_right_click_items_action_oneOf_i1_run_oneOf_i0"></a>2.4.2.1.1. Property `Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 0` | ||
|
|
||
| | | | | ||
| | ------------ | -------- | | ||
| | **Type** | `string` | | ||
| | **Required** | No | | ||
|
|
||
| ###### <a name="settings_right_click_items_action_oneOf_i1_run_oneOf_i1"></a>2.4.2.1.2. Property `Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 1` | ||
|
|
||
| | | | | ||
| | ------------ | ----------------- | | ||
| | **Type** | `array of string` | | ||
| | **Required** | No | | ||
|
|
||
| | | Array restrictions | | ||
| | -------------------- | ------------------ | | ||
| | **Min items** | 1 | | ||
| | **Max items** | N/A | | ||
| | **Items unicity** | False | | ||
| | **Additional items** | False | | ||
| | **Tuple validation** | See below | | ||
|
|
||
| | Each item of this array must be | Description | | ||
| | ------------------------------------------------------------------------------ | ----------- | | ||
| | [item 1 items](#settings_right_click_items_action_oneOf_i1_run_oneOf_i1_items) | | | ||
|
|
||
| ###### <a name="settings_right_click_items_action_oneOf_i1_run_oneOf_i1_items"></a>2.4.2.1.2.1. Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 1 > item 1 items | ||
|
|
||
| | | | | ||
| | ------------ | -------- | | ||
| | **Type** | `string` | | ||
| | **Required** | No | | ||
|
|
Contributor
There was a problem hiding this comment.
Duplicate anchor names and incorrect property paths (same issue as editor file).
The subsections at lines 1549-1580 have the same duplicate anchor and incorrect property path issues as the editor file section. Anchors settings_right_click_items_action_oneOf_i1_run_oneOf_i0 and settings_right_click_items_action_oneOf_i1_run_oneOf_i1 are redefined here, and the section titles refer to right-click actions instead of editor folder properties.
📚 Recommended fix to reference the shared definition
Replace lines 1544-1580 with:
-| One of(Option) |
-| ------------------------------------------------------------------ |
-| [item 0](`#settings_right_click_items_action_oneOf_i1_run_oneOf_i0`) |
-| [item 1](`#settings_right_click_items_action_oneOf_i1_run_oneOf_i1`) |
-
-###### <a name="settings_right_click_items_action_oneOf_i1_run_oneOf_i0"></a>2.4.2.1.1. Property `Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 0`
-
-| | |
-| ------------ | -------- |
-| **Type** | `string` |
-| **Required** | No |
-
-###### <a name="settings_right_click_items_action_oneOf_i1_run_oneOf_i1"></a>2.4.2.1.2. Property `Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 1`
-
-| | |
-| ------------ | ----------------- |
-| **Type** | `array of string` |
-| **Required** | No |
-
-| | Array restrictions |
-| -------------------- | ------------------ |
-| **Min items** | 1 |
-| **Max items** | N/A |
-| **Items unicity** | False |
-| **Additional items** | False |
-| **Tuple validation** | See below |
-
-| Each item of this array must be | Description |
-| ------------------------------------------------------------------------------ | ----------- |
-| [item 1 items](`#settings_right_click_items_action_oneOf_i1_run_oneOf_i1_items`) | |
-
-###### <a name="settings_right_click_items_action_oneOf_i1_run_oneOf_i1_items"></a>2.4.2.1.2.1. Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 1 > item 1 items
-
-| | |
-| ------------ | -------- |
-| **Type** | `string` |
-| **Required** | No |
+| **Same definition as** | [run](`#settings_right_click_items_action_oneOf_i1_run`) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/src/content/docs/dev/reference/schema.mdx` around lines 1528 - 1581, The
subsection documentation for the `run` property under `editor > folder` contains
duplicate anchor names and incorrect property path references that point to
`right_click` instead of the correct `editor > folder` path. Rename the
duplicate anchors `settings_right_click_items_action_oneOf_i1_run_oneOf_i0` and
`settings_right_click_items_action_oneOf_i1_run_oneOf_i1` to use the correct
naming pattern reflecting the editor folder property path (such as
`settings_editor_folder_run_oneOf_i0` and
`settings_editor_folder_run_oneOf_i1`), and update all section titles in lines
1549-1580 to correctly reference `Rovr Config > settings > editor > folder >
run` instead of the incorrect right_click property path references.
Comment on lines
+1614
to
+1667
| | [run](#settings_editor_bulk_editor_run) | object | Editor to use for bulk editing | | ||
| | [rename_show_as_mapping](#settings_editor_bulk_editor_rename_show_as_mapping) | boolean | When doing a bulk rename, show as a mapping of \`<old> ➔ <new>\` instead of just showing only the new names | | ||
| | [shell](#settings_editor_bulk_editor_shell) | boolean | Whether the command to run is a shell command that needs to be run in a shell (required if you want to do complicated things like piping in your editor command) | | ||
|
|
||
| ##### <a name="settings_editor_bulk_editor_run"></a>2.4.3.1. Property `Rovr Config > settings > editor > bulk_editor > run` | ||
|
|
||
| | | | | ||
| | ------------ | ----------- | | ||
| | **Type** | `string` | | ||
| | **Required** | No | | ||
| | **Default** | `"$EDITOR"` | | ||
| | | | | ||
| | ------------------------- | ----------------- | | ||
| | **Type** | `combining` | | ||
| | **Required** | No | | ||
| | **Additional properties** | Any type allowed | | ||
| | **Default** | `"$EDITOR"` | | ||
| | **Defined in** | #/definitions/run | | ||
|
|
||
| **Description:** Editor to use for bulk editing | ||
|
|
||
| | One of(Option) | | ||
| | ------------------------------------------------------------------ | | ||
| | [item 0](#settings_right_click_items_action_oneOf_i1_run_oneOf_i0) | | ||
| | [item 1](#settings_right_click_items_action_oneOf_i1_run_oneOf_i1) | | ||
|
|
||
| ###### <a name="settings_right_click_items_action_oneOf_i1_run_oneOf_i0"></a>2.4.3.1.1. Property `Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 0` | ||
|
|
||
| | | | | ||
| | ------------ | -------- | | ||
| | **Type** | `string` | | ||
| | **Required** | No | | ||
|
|
||
| ###### <a name="settings_right_click_items_action_oneOf_i1_run_oneOf_i1"></a>2.4.3.1.2. Property `Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 1` | ||
|
|
||
| | | | | ||
| | ------------ | ----------------- | | ||
| | **Type** | `array of string` | | ||
| | **Required** | No | | ||
|
|
||
| | | Array restrictions | | ||
| | -------------------- | ------------------ | | ||
| | **Min items** | 1 | | ||
| | **Max items** | N/A | | ||
| | **Items unicity** | False | | ||
| | **Additional items** | False | | ||
| | **Tuple validation** | See below | | ||
|
|
||
| | Each item of this array must be | Description | | ||
| | ------------------------------------------------------------------------------ | ----------- | | ||
| | [item 1 items](#settings_right_click_items_action_oneOf_i1_run_oneOf_i1_items) | | | ||
|
|
||
| ###### <a name="settings_right_click_items_action_oneOf_i1_run_oneOf_i1_items"></a>2.4.3.1.2.1. Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 1 > item 1 items | ||
|
|
||
| | | | | ||
| | ------------ | -------- | | ||
| | **Type** | `string` | | ||
| | **Required** | No | | ||
|
|
Contributor
There was a problem hiding this comment.
Duplicate anchor names and incorrect property paths (same issue as other editor sections).
The subsections at lines 1635-1666 repeat the duplicate anchor and incorrect property path pattern. The anchors and section titles should either be unique to the bulk_editor context or replaced with a "Same definition as" reference.
📚 Recommended fix to reference the shared definition
Replace lines 1630-1666 with:
-| One of(Option) |
-| ------------------------------------------------------------------ |
-| [item 0](`#settings_right_click_items_action_oneOf_i1_run_oneOf_i0`) |
-| [item 1](`#settings_right_click_items_action_oneOf_i1_run_oneOf_i1`) |
-
-###### <a name="settings_right_click_items_action_oneOf_i1_run_oneOf_i0"></a>2.4.3.1.1. Property `Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 0`
-
-| | |
-| ------------ | -------- |
-| **Type** | `string` |
-| **Required** | No |
-
-###### <a name="settings_right_click_items_action_oneOf_i1_run_oneOf_i1"></a>2.4.3.1.2. Property `Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 1`
-
-| | |
-| ------------ | ----------------- |
-| **Type** | `array of string` |
-| **Required** | No |
-
-| | Array restrictions |
-| -------------------- | ------------------ |
-| **Min items** | 1 |
-| **Max items** | N/A |
-| **Items unicity** | False |
-| **Additional items** | False |
-| **Tuple validation** | See below |
-
-| Each item of this array must be | Description |
-| ------------------------------------------------------------------------------ | ----------- |
-| [item 1 items](`#settings_right_click_items_action_oneOf_i1_run_oneOf_i1_items`) | |
-
-###### <a name="settings_right_click_items_action_oneOf_i1_run_oneOf_i1_items"></a>2.4.3.1.2.1. Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 1 > item 1 items
-
-| | |
-| ------------ | -------- |
-| **Type** | `string` |
-| **Required** | No |
+| **Same definition as** | [run](`#settings_right_click_items_action_oneOf_i1_run`) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/src/content/docs/dev/reference/schema.mdx` around lines 1614 - 1667, The
anchor names and property paths in the subsections describing the bulk_editor
run property (starting with the section for settings_editor_bulk_editor_run and
continuing through the oneOf items) are incorrect and duplicate the right_click
context rather than reflecting the bulk_editor context. Replace the detailed
subsection documentation (lines 1630-1666) that enumerate the oneOf options with
a reference pattern that points to the shared run definition, similar to how
other shared schema properties are documented, rather than repeating the full
schema structure with incorrect anchor names and property paths.
| "properties": { | ||
| "run": { | ||
| "type": "string", | ||
| "$ref": "#/definitions/run", |
Contributor
There was a problem hiding this comment.
Right-click run now allows lists, but the runtime path still expects a string.
At Line 44, the schema allows run: list[str], but the right-click execution path still feeds event.option.action["run"] into expand_command(command: str). A list value will fail when string methods are applied, so this can break right-click actions at runtime when users adopt the new list form.
Could we align the right-click runtime path with this schema change (either normalise list-form commands first, or support str | list[str] in expansion)?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/rovr/config/schema.json` at line 44, The schema now allows run to be a
list of strings, but the right-click execution path still expects a single
string value. In the right-click runtime handler where
event.option.action["run"] is passed to expand_command, update the code to
handle both string and list types. Either normalize the list form to a string
before calling expand_command (by joining list elements), or modify the
expand_command function signature to accept str | list[str] and handle both
cases internally. This will ensure the runtime behavior aligns with the updated
schema definition.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rovr/core/file_list_right_click_menu.py`:
- Around line 295-297: The placeholder requirement checks earlier in the menu
disabling logic don't properly scan individual elements within list-type
commands for embedded placeholders like file=${highlighted_file}, since they
only check whole list elements. Before the expand_command call that processes
event.option.action["run"], add normalization logic that converts the run value
(which can be either a string or list[str]) into a consistent parts format so
that placeholder detection works identically for both command types, ensuring
any placeholders embedded within list elements are caught by the earlier
validation checks.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac943948-bcd9-491b-a2ee-8cf00f1eb6e7
📒 Files selected for processing (6)
AGENTS.mdsrc/rovr/core/file_list.pysrc/rovr/core/file_list_right_click_menu.pysrc/rovr/functions/config.pysrc/rovr/functions/utils.pysrc/rovr/screens/typed.py
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/rovr/functions/config.py
- src/rovr/core/file_list.py
- src/rovr/functions/utils.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python 3.12+ is required
Use double quotes for string literals
Use 4 spaces for indentation
Use absolute imports only, no relative imports
All functions must have type annotations (ANN rules enforced by ruff)
Usetyfor type checking, not mypy
Do not useif TYPE_CHECKINGblocks for imports
Use CamelCase for class names (e.g.,FileList,Application)
Use snake_case for function and variable names (e.g.,get_filtered_dir_names)
Use UPPER_CASE for constants (e.g.,MAX_ITEMS)
Prefix private functions/variables with underscore (e.g.,_internal_helper)
Use specific exceptions instead of bareexcept:clauses
Usesuppressfrom contextlib for expected errors
Custom exceptions should be defined inrovr/classes/exceptions.py
Avoid adding unnecessary comments; aim for self-documenting code instead
Usingsubprocess.run(shell=True)is allowed with no security implications since the user controls the config
Files:
src/rovr/screens/typed.pysrc/rovr/core/file_list_right_click_menu.py
src/rovr/{components,core,screens}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/rovr/{components,core,screens}/**/*.py: For Textual widgets, inherit withinherit_bindings=Falsefor custom keybindings
Use@on(EventType)decorators oron_eventmethods for event handling in Textual
Use@workdecorator for async background work in Textual, withthread=Truefor blocking tasks
Usecompose()methods for widget composition in Textual
For widget modifications in a thread, usecall_from_thread,call_next,call_later, orcall_after_refreshin Textual
Event handlers in Textual do not need to callsuper()explicitly; prevent bubbling withprevent_default()if needed
Files:
src/rovr/screens/typed.pysrc/rovr/core/file_list_right_click_menu.py
src/rovr/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Avoid creating functions in the same file as
ProcessPoolExecutororProcessto prevent config reloading issues and stdout corruption
Files:
src/rovr/screens/typed.pysrc/rovr/core/file_list_right_click_menu.py
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: NSPC911/rovr
Timestamp: 2026-06-23T05:10:58.859Z
Learning: Follow Conventional Commits with types: `feat`, `fix`, `docs`, `style`, `refactor`, `test`, `chore`
Learnt from: CR
Repo: NSPC911/rovr
Timestamp: 2026-06-23T05:10:58.859Z
Learning: Do not refactor; the project prefers features over refactors
Learnt from: CR
Repo: NSPC911/rovr
Timestamp: 2026-06-23T05:10:58.859Z
Learning: Use `uv` commands; do not use `pip` or `python -m`
Learnt from: CR
Repo: NSPC911/rovr
Timestamp: 2026-06-23T05:10:58.859Z
Learning: Always run `poe check` before committing
Learnt from: CR
Repo: NSPC911/rovr
Timestamp: 2026-06-23T05:10:58.859Z
Learning: Run `poe fmt` to apply formatting fixes suggested by ruff
Learnt from: CR
Repo: NSPC911/rovr
Timestamp: 2026-06-23T05:10:58.859Z
Learning: Run `poe test` before committing to ensure no tests are broken
Learnt from: CR
Repo: NSPC911/rovr
Timestamp: 2026-06-23T05:10:58.859Z
Learning: Commit with `NSPBot911 <176916861+NSPBot911users.noreply.github.com>` to distinguish bot commits; mention provider in commit message with syntax `Used [Provider Name]: [Model Name]`
📚 Learning: 2025-12-11T03:10:54.344Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 167
File: src/rovr/footer/process_container.py:346-346
Timestamp: 2025-12-11T03:10:54.344Z
Learning: In Python/Textual code, when calling checker_wrapper() or any other method that starts workers from within a work(thread=True) decorated method, do not wrap the call in call_from_thread. Starting workers is thread-safe in Textual and the work decorator handles threading. Only UI updates (e.g., DOM modifications) from worker threads require call_from_thread. Apply this guideline to Python files across the codebase where worker-start calls occur; do not wrap such calls with call_from_thread.
Applied to files:
src/rovr/screens/typed.pysrc/rovr/core/file_list_right_click_menu.py
📚 Learning: 2026-01-24T02:10:57.940Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 203
File: src/rovr/first_launch.py:273-295
Timestamp: 2026-01-24T02:10:57.940Z
Learning: In the rovr codebase (Python/Textual), App.notify() is thread-safe and may be called directly from worker threads decorated with work(thread=True) without wrapping in call_from_thread(). Only DOM modifications require call_from_thread from worker threads. Apply this guideline to Python files across the codebase (not just a single file) and avoid unnecessary call_from_thread() wrappers when invoking App.notify() from worker threads.
Applied to files:
src/rovr/screens/typed.pysrc/rovr/core/file_list_right_click_menu.py
📚 Learning: 2026-01-30T13:54:15.110Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 207
File: src/rovr/first_launch.py:96-107
Timestamp: 2026-01-30T13:54:15.110Z
Learning: In Python projects using the Textual framework, async workers decorated with work are automatically cancelled when the parent screen/widget is dismissed or unmounted. Do not add manual cancellation in cleanup/dismiss handlers for these workers; rely on Textual to cancel them. Verify this behavior against the project’s Textual version before relying on it.
Applied to files:
src/rovr/screens/typed.pysrc/rovr/core/file_list_right_click_menu.py
📚 Learning: 2026-02-27T09:15:49.243Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 229
File: src/rovr/functions/system_clipboard.py:143-143
Timestamp: 2026-02-27T09:15:49.243Z
Learning: In Python files that display error messages in toast/notification UI (e.g., ClipboardError and similar exceptions), escape unescaped square brackets in the message so the toast system does not interpret them as markup. Specifically render the content with double backslashes before brackets, e.g. "\\[{type(exc).__name__}]" in the toast description. This guideline applies to all Python files that format and show exception details in toasts across the codebase.
Applied to files:
src/rovr/screens/typed.pysrc/rovr/core/file_list_right_click_menu.py
📚 Learning: 2025-12-11T03:11:24.387Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 167
File: src/rovr/screens/fd_search.py:205-209
Timestamp: 2025-12-11T03:11:24.387Z
Learning: In src/rovr/screens/fd_search.py, FileSearch.FILTER_TYPES should be implemented as a class attribute (not an instance attribute) to persist filter state across all modal instances within the same session. This enables remember/restore of filter settings when the modal is dismissed and reopened. Ensure this behavior is intentional, document the session lifecycle implications, and include tests verifying persistence across modal reopenings, as well as proper reset or isolation when a new session starts. If applicable, consider thread-safety and potential memory growth due to long-lived state.
Applied to files:
src/rovr/screens/typed.pysrc/rovr/core/file_list_right_click_menu.py
📚 Learning: 2025-12-11T03:11:27.833Z
Learnt from: NSPC911
Repo: NSPC911/rovr PR: 167
File: src/rovr/screens/fd_search.py:205-209
Timestamp: 2025-12-11T03:11:27.833Z
Learning: In src/rovr/screens/fd_search.py, document that the FileSearch class uses a class-level FILTER_TYPES dict to persist filter settings across multiple modal invocations within a single application session. Ensure this pattern is intentional by clarifying the lifecycle of the shared state (lifetime of the process/session) and the implications of mutable shared state (thread-safety, potential memory retention). Consider alternatives (per-instance state, dedicated session storage, explicit reset method) and add tests to verify persistence, correctness after modal reopenings, and safe reset behavior. Also mention any framework or runtime constraints that affect shared-class state.
Applied to files:
src/rovr/screens/typed.pysrc/rovr/core/file_list_right_click_menu.py
📚 Learning: 2026-01-03T14:17:18.132Z
Learnt from: lazysegtree
Repo: NSPC911/rovr PR: 184
File: src/rovr/core/preview_container.py:280-283
Timestamp: 2026-01-03T14:17:18.132Z
Learning: In src/rovr/core/preview_container.py, mypy may not recognize that pdfinfo_from_path's poppler_path parameter accepts None even though the library signature is Optional[Union[str, pathlib.PurePath]]. If you encounter a type error, explicitly cast the argument to Optional[Union[str, pathlib.PurePath]] before passing it (e.g., from typing import cast; pdfinfo_from_path(path, poppler_path=cast(Optional[Union[str, pathlib.PurePath]], None)) or cast the argument to the expected type). This documents the typing limitation and provides a repeatable workaround for similar cases.
Applied to files:
src/rovr/screens/typed.pysrc/rovr/core/file_list_right_click_menu.py
🔇 Additional comments (1)
src/rovr/screens/typed.py (1)
25-27: LGTM!
Owner
Author
|
@sourcery-ai resolve |
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 security issues, 4 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- In
load_config, the non-list branch forruntreatsexpanded_runas an iterable of characters (usingany(part in (...) for part in expanded_run)andexpanded_run[0]), which will mis-detect unresolved$EDITORplaceholders and can even break whenexpanded_runis empty; this logic should operate on the full string rather than character-by-character. - The opener fallback logic in
file_listno longer validates the executable withwhich(runner[0])before callingrun_command, so a missing binary will now raise immediately instead of falling through to the next opener; consider preserving the previous graceful fallback behaviour or handling failures explicitly aroundrun_command.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `load_config`, the non-list branch for `run` treats `expanded_run` as an iterable of characters (using `any(part in (...) for part in expanded_run)` and `expanded_run[0]`), which will mis-detect unresolved `$EDITOR` placeholders and can even break when `expanded_run` is empty; this logic should operate on the full string rather than character-by-character.
- The opener fallback logic in `file_list` no longer validates the executable with `which(runner[0])` before calling `run_command`, so a missing binary will now raise immediately instead of falling through to the next opener; consider preserving the previous graceful fallback behaviour or handling failures explicitly around `run_command`.
## Individual Comments
### Comment 1
<location path="src/rovr/functions/config.py" line_range="643-646" />
<code_context>
+ token in raw_run for token in ("$EDITOR", "${EDITOR}", "%EDITOR%")
+ ):
+ expanded_run = ""
+ unresolved_editor = any(
+ part in ("$EDITOR", "${EDITOR}", "%EDITOR%") for part in expanded_run
+ )
+ if not expanded_run or unresolved_editor or not expanded_run[0]:
+ expanded_run = editor()
+ config_dict["settings"]["editor"][key]["run"] = expanded_run
</code_context>
<issue_to_address>
**issue (bug_risk):** The unresolved $EDITOR detection iterates characters instead of tokens, so it never triggers as intended.
Here `expanded_run` is a string, so `any(part in (...) for part in expanded_run)` iterates over characters, not tokens, and will never match `$EDITOR`-style placeholders. If you want to detect unresolved placeholders, you should search substrings (e.g. `any(token in expanded_run for token in (...))`) or reuse the previous token-based logic. Also, once you’ve checked `not expanded_run`, the additional `not expanded_run[0]` condition is unnecessary.
</issue_to_address>
### Comment 2
<location path="src/rovr/core/file_list.py" line_range="427-436" />
<code_context>
- )
- opened = True
- break
+ runner = opener if isinstance(opener, str) else opener["run"]
+ run_type = (
+ ("orphan" if opener.get("orphan", True) else "suspend")
+ if isinstance(opener, dict)
+ else "orphan"
+ )
+ shell = (
+ opener.get("shell", True)
+ if isinstance(opener, dict)
+ else True
+ )
+ proc = utils.run_command(
+ self.app,
+ utils.command(runner, target_path),
</code_context>
<issue_to_address>
**issue:** Dropping the `which` check for openers can lead to uncaught FileNotFoundError for orphaned commands.
`which(runner[0])` previously ensured the opener existed before running it. Now `run_command` is always invoked, so an orphan opener with a missing executable will raise an uncaught `FileNotFoundError` from `Popen`. Please guard against this (e.g., reintroduce a presence check or explicitly handle `FileNotFoundError`) so orphan processes can't crash the file listing path.
</issue_to_address>
### Comment 3
<location path="docs/src/content/docs/dev/reference/schema.mdx" line_range="85-87" />
<code_context>
+ - [2.4.1.1.1. Property `Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 0`](#settings_right_click_items_action_oneOf_i1_run_oneOf_i0)
</code_context>
<issue_to_address>
**issue:** Editor schema subsections reuse right-click anchors and labels, which is confusing and likely incorrect.
This entry lives under the editor `file > run` hierarchy, but its link text and anchor still reference the right-click path. That misleads readers and causes anchor reuse across unrelated sections. Please introduce editor-specific anchors and labels that reflect the editor context (while noting they share the same underlying `run` definition).
</issue_to_address>
### Comment 4
<location path="AGENTS.md" line_range="166" />
<code_context>
- Commit with `NSPBot911 <176916861+NSPBot911@users.noreply.github.com>` to better distinguish between human and bot commits in the history.
NEVER co-author with your provider (do not `Claude Opus 4.6 <noreply@anthropic.com>`)
You can, however, mention the provider used in the commit message, but you must use a syntax of `Used [Provider Name]: [Model Name]`
+- I cannot make any changes to docs/src/content/docs/reference/schema.mdx or docs/src/content/docs/dev/reference/schema.mdx, they are auto-generated by `poe gen-schema`.
### Interesting Textual Patterns
</code_context>
<issue_to_address>
**issue (typo):** Consider fixing the comma splice between the file list and the auto-generation note.
The phrase after the comma ("they are auto-generated by `poe gen-schema`") is an independent clause, so the comma creates a comma splice. Please either split into two sentences or add a coordinating conjunction (e.g., "..., and they are auto-generated...") to fix the grammar.
```suggestion
- I cannot make any changes to docs/src/content/docs/reference/schema.mdx or docs/src/content/docs/dev/reference/schema.mdx, and they are auto-generated by `poe gen-schema`.
```
</issue_to_address>
### Comment 5
<location path="src/rovr/functions/utils.py" line_range="159-166" />
<code_context>
return subprocess.Popen(
command,
stdin=subprocess.DEVNULL,
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
creationflags=subprocess.CREATE_NEW_PROCESS_GROUP
| subprocess.DETACHED_PROCESS,
shell=shell,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 6
<location path="src/rovr/functions/utils.py" line_range="169-176" />
<code_context>
return subprocess.Popen(
command,
stdin=subprocess.DEVNULL,
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
start_new_session=True,
shell=shell,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+643
to
+646
| unresolved_editor = any( | ||
| part in ("$EDITOR", "${EDITOR}", "%EDITOR%") for part in expanded_run | ||
| ) | ||
| if not expanded_run or unresolved_editor or not expanded_run[0]: |
There was a problem hiding this comment.
issue (bug_risk): The unresolved $EDITOR detection iterates characters instead of tokens, so it never triggers as intended.
Here expanded_run is a string, so any(part in (...) for part in expanded_run) iterates over characters, not tokens, and will never match $EDITOR-style placeholders. If you want to detect unresolved placeholders, you should search substrings (e.g. any(token in expanded_run for token in (...))) or reuse the previous token-based logic. Also, once you’ve checked not expanded_run, the additional not expanded_run[0] condition is unnecessary.
Comment on lines
+85
to
+87
| - [2.4.1.1.1. Property `Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 0`](#settings_right_click_items_action_oneOf_i1_run_oneOf_i0) | ||
| - [2.4.1.1.2. Property `Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 1`](#settings_right_click_items_action_oneOf_i1_run_oneOf_i1) | ||
| - [2.4.1.1.2.1. Rovr Config > settings > right_click > right_click items > action > oneOf > item 1 > run > oneOf > item 1 > item 1 items](#settings_right_click_items_action_oneOf_i1_run_oneOf_i1_items) |
There was a problem hiding this comment.
issue: Editor schema subsections reuse right-click anchors and labels, which is confusing and likely incorrect.
This entry lives under the editor file > run hierarchy, but its link text and anchor still reference the right-click path. That misleads readers and causes anchor reuse across unrelated sections. Please introduce editor-specific anchors and labels that reflect the editor context (while noting they share the same underlying run definition).
| - Commit with `NSPBot911 <176916861+NSPBot911@users.noreply.github.com>` to better distinguish between human and bot commits in the history. | ||
| NEVER co-author with your provider (do not `Claude Opus 4.6 <noreply@anthropic.com>`) | ||
| You can, however, mention the provider used in the commit message, but you must use a syntax of `Used [Provider Name]: [Model Name]` | ||
| - I cannot make any changes to docs/src/content/docs/reference/schema.mdx or docs/src/content/docs/dev/reference/schema.mdx, they are auto-generated by `poe gen-schema`. |
There was a problem hiding this comment.
issue (typo): Consider fixing the comma splice between the file list and the auto-generation note.
The phrase after the comma ("they are auto-generated by poe gen-schema") is an independent clause, so the comma creates a comma splice. Please either split into two sentences or add a coordinating conjunction (e.g., "..., and they are auto-generated...") to fix the grammar.
Suggested change
| - I cannot make any changes to docs/src/content/docs/reference/schema.mdx or docs/src/content/docs/dev/reference/schema.mdx, they are auto-generated by `poe gen-schema`. | |
| - I cannot make any changes to docs/src/content/docs/reference/schema.mdx or docs/src/content/docs/dev/reference/schema.mdx, and they are auto-generated by `poe gen-schema`. |
Comment on lines
159
to
166
| return subprocess.Popen( | ||
| command, | ||
| stdin=subprocess.DEVNULL, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| stdout=subprocess.DEVNULL, | ||
| stderr=subprocess.DEVNULL, | ||
| creationflags=subprocess.CREATE_NEW_PROCESS_GROUP | ||
| | subprocess.DETACHED_PROCESS, | ||
| shell=shell, |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
Comment on lines
169
to
176
| return subprocess.Popen( | ||
| command, | ||
| stdin=subprocess.DEVNULL, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| stdout=subprocess.DEVNULL, | ||
| stderr=subprocess.DEVNULL, | ||
| start_new_session=True, | ||
| shell=shell, | ||
| ) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
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
1 participant
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.
so like run commands can now be
["echo", "hello"]instead of"echo hello", however it will automatically switch to list if shell is FalseSummary by Sourcery
Allow run commands in configuration to be specified as either strings or lists and propagate support throughout command execution and UI.
New Features:
Enhancements:
Documentation:
Summary by CodeRabbit
Release Notes
New Features
runcommands for right-click actions, editor settings (file/folder/bulk), and file openers now accept either a single command or multiple commands as an array.Bug Fixes
runis provided as a list or string.Documentation
run(oneOf) format and nested options.