Add cross-platform install script - #3
Conversation
Adds install.sh — a single script that works on macOS, Linux, and Windows (Git Bash/MSYS). Auto-detects OS, builds with webpack, uses --legacy-peer-deps for Angular conflicts. Updates README with one-liner and manual install instructions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
On MINGW/CYGWIN/MSYS, package with npm pack then install the .tgz instead of symlinking — matching README recommendation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
a-rank
left a comment
There was a problem hiding this comment.
Review: Add cross-platform install script
Thanks for the contribution! The install script is a nice convenience. However, there are a few concerns that should be addressed before merging.
Security: curl | bash one-liner points to the contributor's fork
The README one-liner pipes a script from cdn.jsdelivr.net/gh/GodSpoon/tabby-scroll-fix@add-install-script/install.sh directly into bash. This has two problems:
- It points to the contributor's fork, not the upstream repo. After merging, the upstream repo would be directing users to execute a script hosted in a third-party fork — which the contributor can change at any time without review. This is a supply chain risk.
curl | bashis inherently risky and may not be appropriate for a small plugin like this. If kept, the URL should at minimum point to the upstream repo (e.g.,a-rank/tabby-scroll-fix@master).
Recommendation: Either remove the one-liner entirely, or change the URL to point to the upstream repo after merging.
Script issues
-
install.shwon't work when piped from curl. When run ascurl ... | bash,$0isbashanddirname "$0"resolves to/usr/binor similar — not the plugin source directory. The script assumes it's being run from a local clone (it references$SOURCE_PATH/dist,$SOURCE_PATH/package.json, etc.), so the one-liner will fail or produce confusing errors. Either the script needs to clone the repo first when detecting it's being piped, or the one-liner should be removed. -
Tarball rename on Windows is fragile. The
mv tabby-scroll-fix-*.tgz "$TARBALL" 2>/dev/null || mv tabby-scroll-fix.tgz "$TARBALL" 2>/dev/null || truechain silently swallows errors via|| true. If bothmvcommands fail, the script continues and tries tonpm installa nonexistent tarball. This should fail explicitly. -
No
package.jsoninitialization in$PLUGIN_DIR. The manual install instructions in the README includenpm init -ybeforenpm install, but the script skips this step. If the plugins directory is fresh (nopackage.json),npm installmay behave unexpectedly.
Minor
- The README changes demote "Build" to just part of "Install (manual)" which loses the subsection header structure. Consider keeping
### Buildand### Install into Tabbyas sub-headers under "Install (manual)".
Summary
The core idea is useful, but the curl | bash one-liner pointing to a fork is a blocker, and the script doesn't actually work when piped from curl. Please address these before merging.
Generated by Claude Code
Summary
install.sh— one script for macOS, Linux, Windows (Git Bash/MSYS)--legacy-peer-depsfor Angular conflicts, tarball install on Windows🤖 Generated with Claude Code