Repository navigation
Conversation
a1e36df to
146279a
Compare
|
I wanna see this merged, but I found some bugs, and I'll need some time to compile them into good feedback. I hope it won't take long. |
Theoreticallyhugo
left a comment
Theoreticallyhugo
left a comment
There was a problem hiding this comment.
I commented on one line, but this applies to all other lines like it.
I am also not done yet, but probably won't be able to post the rest of the review today.
Since you're using AI as co-author, you might wanna ask it to check your PR for bugs and such.
| if [[ -x "${current_dir}/${script}" ]]; then | ||
| IFS=' ' read -r -a colors <<<$(get_tmux_option "@dracula-custom-plugin-colors" "cyan dark_gray") | ||
| script="#($current_dir/${script})" | ||
| script="#($current_dir/${script} #{session_name})" |
There was a problem hiding this comment.
Passing session_name like this seems to word-split, meaning the two sessions "test proj1" and "test proj2" clash.
Passing as #{q:session_name} should fix this problem for the most part.
q:is specific to tmux.
It is roughly documented in (the manpages](https://man.openbsd.org/tmux#FORMATS).
At the time of writing, the following characters are escaped:
| & ; < > ( ) { } $\ " ' * ? [ # space = % \n \t`
Find this on line 4462 in the tmux source
Also, from what i can tell, not quoting with q: means the session name is evaluated. This is somewhat dangerous. For me running tmux rename-session 'x$(touch /tmp/PWNED)y' actually places a file there. So, naming your session after removing the french language pack could cause tears.
There was a problem hiding this comment.
You're right. I went with #{q:session_id} which is safer and fixes other issues as you noted.
Also dropped list-panes -f that doesn't exists before nuxt 3.2
Theoreticallyhugo
left a comment
Theoreticallyhugo
left a comment
There was a problem hiding this comment.
Found some things that need fixing in this PR, and some that I'll put into issues.
Let me know if you got any questions or objections.
| local session="$1" | ||
| if [ -n "$session" ]; then | ||
| tmux list-panes -t "$session" -f '#{pane_active}' -F "#{pane_current_path}" 2>/dev/null | ||
| else | ||
| tmux list-panes -f '#{pane_active}' -F "#{pane_current_path}" 2>/dev/null | ||
| fi |
There was a problem hiding this comment.
Since this requires the session_name as first argument to the script, this should be documented (at the start of the script i'd argue, but i'm open to other ideas)
There was a problem hiding this comment.
Done, documented in usage header(s)
There was a problem hiding this comment.
If the path is empty (which can happen for a variety of semi rare reasons), this widget displays a bare ~. This is a bug generally unrelated to this PR, because /tmp is changed to ~/tmp too. So, I'll have to open an issue/ PR for that.
There was a problem hiding this comment.
Nevertheless it might be good to think about failed path resolutions.
The following two are probably acceptable failure modes
- Pane is on path that has been deleted
- cwd just shows the old path, and others tend to just fail. I guess thats fine
- script is run with session name resolved earlier, but session name has changed since (a very rare race)
- this can be fixed by passing session_id instead
- if this ever happens, it should be visible til the next pane update only, so i dont think this is really relevant.
However, we don't get feedback if we expect to get your new fix, but $1 is empty or never passed, because we fall back to the old, unfixed, method without feedback. We might wanna document this in a docstring in a the function.
There was a problem hiding this comment.
Done, fallback kept (if script is run by hand, without dracula.sh passing $1) and documented.
| if [[ -x "${current_dir}/${script}" ]]; then | ||
| IFS=' ' read -r -a colors <<<$(get_tmux_option "@dracula-custom-plugin-colors" "cyan dark_gray") | ||
| script="#($current_dir/${script})" | ||
| script="#($current_dir/${script} #{session_name})" |
There was a problem hiding this comment.
This change passes the session name to all custom scripts as $1.
This contract change must be documented in docs/CONFIG.md
There was a problem hiding this comment.
Done. Documented in the custom:script-name section.
146279a to
99f8336
Compare
99f8336 to
9453be9
Compare
…t current active pane Assisted-by: Claude Code (Claude Sonnet 5, Claude Opus 5.5)
9453be9 to
d56bcc7
Compare
2 participants
git.sh,cwd.sh, etc, are trying to display data about the current active pane. However, with how it is handled today, having two different sessions live with two different panes would show erroneous data in one of them since the scrips cannot know in which session they are running.It can be fixed by using
#{session_name}to find the active pane in the right session.Assisted-by: Claude Sonnet 5