Skip to content

fix(RemoteTreeBrowser): add sh -c prefix#64

Closed
YaQia wants to merge 9 commits into
inhesrom:masterfrom
YaQia:master
Closed

fix(RemoteTreeBrowser): add sh -c prefix#64
YaQia wants to merge 9 commits into
inhesrom:masterfrom
YaQia:master

Conversation

@YaQia
Copy link
Copy Markdown
Contributor

@YaQia YaQia commented Feb 6, 2026

I use fish as a default shell, so add bash -c is needed for compatibility.

Copilot AI review requested due to automatic review settings February 6, 2026 17:13
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request aims to fix compatibility issues for users who have fish shell as their default shell by wrapping remote SSH commands in bash -c to ensure POSIX-compatible shell syntax works correctly. The fish shell doesn't support bash-specific syntax like ${var#pattern}, which is used in the directory browsing commands.

Changes:

  • Added bash -c wrapper to all SSH commands that list remote directories
  • Updated comments to explain the reason for using bash -c
  • Modified command string formatting across multiple functions in tree_browser.lua and browse.lua

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.

File Description
lua/async-remote-write/tree_browser.lua Added bash -c wrapper to the load_directory function's SSH command
lua/async-remote-write/browse.lua Added bash -c wrapper to five different directory browsing functions (warm_single_directory, browse_remote_directory, browse_remote_level_based, load_directory_for_tree, load_directory_v2)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lua/async-remote-write/tree_browser.lua Outdated
Comment thread lua/async-remote-write/browse.lua Outdated
Comment thread lua/async-remote-write/tree_browser.lua Outdated
Comment thread lua/async-remote-write/browse.lua Outdated
Comment thread lua/async-remote-write/browse.lua Outdated
Comment thread lua/async-remote-write/tree_browser.lua Outdated
Comment thread lua/async-remote-write/browse.lua Outdated
Comment thread lua/async-remote-write/browse.lua Outdated
Comment thread lua/async-remote-write/browse.lua Outdated
YaQia added 2 commits February 7, 2026 01:31
1. replace bash -c with sh -c, it should work natively.
2. give shell script strings a better format.
@YaQia YaQia requested a review from Copilot February 6, 2026 17:50
@YaQia YaQia changed the title fix(RemoteTreeBrowser): add bash -c prefix fix(RemoteTreeBrowser): add sh -c prefix Feb 6, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lua/async-remote-write/tree_browser.lua Outdated
Comment thread lua/async-remote-write/browse.lua Outdated
Comment thread lua/async-remote-write/browse.lua Outdated
Comment thread lua/async-remote-write/tree_browser.lua Outdated
Comment thread lua/async-remote-write/tree_browser.lua Outdated
Comment thread lua/async-remote-write/browse.lua Outdated
Comment thread lua/async-remote-write/browse.lua Outdated
Comment thread lua/async-remote-write/browse.lua Outdated
Comment thread lua/async-remote-write/browse.lua Outdated
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lua/async-remote-write/browse.lua Outdated
Comment thread lua/async-remote-write/browse.lua Outdated
Comment thread tests/test_ssh_command_escaping.lua
Comment thread lua/async-remote-write/browse.lua Outdated
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lua/async-remote-write/browse.lua Outdated
Comment thread lua/async-remote-write/browse.lua Outdated
Comment thread lua/async-remote-write/browse.lua Outdated
Copy link
Copy Markdown
Contributor Author

@YaQia YaQia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completed. Don't ask me to improve anything not related to the original problem anymore.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@YaQia
Copy link
Copy Markdown
Contributor Author

YaQia commented Feb 6, 2026

@inhesrom Hey, I think this is ready to be merged.

PR Summary: Fix RemoteTreeBrowser for Fish Shell Compatibility

Problem

RemoteTreeBrowser fails with "Failed to load initial tree" when the remote server uses fish shell as the default shell. The shell commands used POSIX/Bash-specific syntax (e.g., ${f#./}) that fish doesn't support.

Solution

Wrap all shell commands with sh -c '...' to ensure execution in a POSIX-compatible shell, regardless of the user's login shell.

New API

-- lua/async-remote-write/ssh_utils.lua
ssh_utils.build_list_dir_cmd(path, opts)
-- opts.sorted: boolean (default: true)

Key Change

Before:

local cmd = 'cd ' .. path .. ' && find . -maxdepth 1 | while read f; do ...'

After:

local cmd = ssh_utils.build_list_dir_cmd(path)
-- Generates: sh -c '...' _ '/path/to/dir'

@inhesrom inhesrom self-assigned this Feb 7, 2026
@inhesrom
Copy link
Copy Markdown
Owner

inhesrom commented Feb 7, 2026

@YaQia thanks for the submission, will be reviewing soon.

@inhesrom
Copy link
Copy Markdown
Owner

inhesrom commented Feb 7, 2026

Given that this is a change to the core functionality, can we start by merging this into a test branch for me to pull down and do some manual testing with?

Please open another PR into the branch: add-sh-prefix I have created. I will merge that PR and then do some testing on that branch.

Thanks again for submitting this, it is very needed!

@YaQia
Copy link
Copy Markdown
Contributor Author

YaQia commented Feb 8, 2026

OK, see #65

@YaQia YaQia closed this Feb 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants