Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe install script improves distro-family detection (handles space-separated ID_LIKE and more derivatives), adds a Linux sudo preflight check, standardizes package-manager invocations and error paths, refactors macOS Homebrew shellenv handling, and adjusts COMMON_DEPS so macOS excludes zsh while ensuring a final unconditional COMMON_DEPS install. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Installer as install.sh
participant Sudo as sudo
participant PM as PackageManager
participant Brew as Homebrew
participant FS as Filesystem
User->>Installer: run install.sh
Installer->>Installer: detect OS / resolve distro family (parse ID_LIKE)
alt Linux
Installer->>Sudo: check existence
Sudo-->>Installer: present / absent
Installer->>PM: run install_pkg (COMMON_DEPS + distro-specific)
PM-->>Installer: install result
else macOS
Installer->>Brew: evaluate `brew shellenv`
Brew-->>Installer: env and prefix
Installer->>Brew: install_pkg (COMMON_DEPS without zsh)
Brew-->>Installer: install result
end
Installer->>FS: install fonts/configs
Installer-->>User: finish (success / error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
install.sh (1)
242-242:⚠️ Potential issue | 🟡 MinorInconsistent:
-yis still used withapt-get updatehere.Line 113 correctly removed
-yfromapt-get update(with a comment noting it's invalid), but this line still uses-ywith update. Apply the same fix for consistency.Proposed fix
- sudo apt-get update -qq -y > /dev/null && sudo apt-get install -qq -y eza > /dev/null + sudo apt-get update -qq > /dev/null && sudo apt-get install -qq -y eza > /dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` at line 242, Update the shell command that currently reads "sudo apt-get update -qq -y > /dev/null && sudo apt-get install -qq -y eza > /dev/null" to remove the invalid "-y" from the apt-get update invocation (so only the install uses "-y"); keep the "-qq" and redirections intact and ensure the resulting command is "sudo apt-get update -qq > /dev/null && sudo apt-get install -qq -y eza > /dev/null".
🧹 Nitpick comments (1)
install.sh (1)
121-123: Consider-Sinstead of-Syto avoid partial upgrade risk.Using
pacman -Sywithout-ucan lead to partial upgrades, which is discouraged in the Arch ecosystem. If the goal is simply to install packages (assuming the user's system is already up-to-date), consider using-Salone. If you intentionally want to refresh the database, the current approach is acceptable but carries some risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 121 - 123, The pacman invocation uses "-Sy" which can cause partial-upgrade risk; update the sudo pacman command that installs "${pkgs[@]}" (the line starting with sudo pacman -Sy -q --noconfirm --needed) to use "-S" instead of "-Sy" if you only intend to install packages, or explicitly use "-Syu" when you intend to refresh and upgrade the system; ensure the replacement preserves the existing flags (-q --noconfirm --needed) and the failure handling block around that command.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Around line 100-104: The error message in the sudo availability check (the
conditional using OS_TYPE and command -v sudo) contradicts the script's earlier
refusal to run as root (unless CI_ENV=true); change the echo to only recommend
installing sudo or configuring CI_ENV=true for CI runs (remove "or run as
root"), or alternatively update the logic to allow root by checking UID==0 and
proceeding when appropriate; specifically modify the sudo-check block that
references OS_TYPE and CI_ENV so the message and behavior are consistent with
the root-rejection logic elsewhere.
---
Outside diff comments:
In `@install.sh`:
- Line 242: Update the shell command that currently reads "sudo apt-get update
-qq -y > /dev/null && sudo apt-get install -qq -y eza > /dev/null" to remove the
invalid "-y" from the apt-get update invocation (so only the install uses "-y");
keep the "-qq" and redirections intact and ensure the resulting command is "sudo
apt-get update -qq > /dev/null && sudo apt-get install -qq -y eza > /dev/null".
---
Nitpick comments:
In `@install.sh`:
- Around line 121-123: The pacman invocation uses "-Sy" which can cause
partial-upgrade risk; update the sudo pacman command that installs "${pkgs[@]}"
(the line starting with sudo pacman -Sy -q --noconfirm --needed) to use "-S"
instead of "-Sy" if you only intend to install packages, or explicitly use
"-Syu" when you intend to refresh and upgrade the system; ensure the replacement
preserves the existing flags (-q --noconfirm --needed) and the failure handling
block around that command.
| # Verify sudo is available on Linux before attempting privileged installs | ||
| if [[ "$OS_TYPE" == "linux" ]] && ! command -v sudo &>/dev/null; then | ||
| echo -e "${RED}sudo is required but not found. Please install sudo or run as root.${NC}" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Contradictory error message regarding root execution.
The message suggests "run as root" as an alternative, but the script explicitly rejects root execution at lines 27-31 (unless CI_ENV=true). This could confuse users on minimal systems without sudo.
Proposed fix
if [[ "$OS_TYPE" == "linux" ]] && ! command -v sudo &>/dev/null; then
- echo -e "${RED}sudo is required but not found. Please install sudo or run as root.${NC}"
+ echo -e "${RED}sudo is required but not found. Please install sudo first.${NC}"
exit 1
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 100 - 104, The error message in the sudo
availability check (the conditional using OS_TYPE and command -v sudo)
contradicts the script's earlier refusal to run as root (unless CI_ENV=true);
change the echo to only recommend installing sudo or configuring CI_ENV=true for
CI runs (remove "or run as root"), or alternatively update the logic to allow
root by checking UID==0 and proceeding when appropriate; specifically modify the
sudo-check block that references OS_TYPE and CI_ENV so the message and behavior
are consistent with the root-rejection logic elsewhere.
1 participant
Summary by CodeRabbit
Bug Fixes
Improvements