Random improvements#100
Conversation
WalkthroughThe pull request migrates the tool installer from Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c489911e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| if [ ! -f "$HOME/.local/bin/fish" ]; then | ||
| python $HERE/scripts/cmd_install.py fish | ||
| python3 $HERE/scripts/cmd_install.py fish |
There was a problem hiding this comment.
On a fresh HOME where ~/.local/bin/fish does not exist, this branch is taken, but scripts/cmd_install.py was deleted in the same commit, so setup exits before installing fish. Point this at bin/cmd-install or the symlink created above instead.
Useful? React with 👍 / 👎.
|
|
||
| Python port of `xdg_config/fish/functions/cmd_install.fish`. | ||
| """ | ||
| #!/usr/bin/env python3 |
There was a problem hiding this comment.
This commit removes the cmd_install fish function and only adds the cmd-install executable, but xdg_config/fish/functions/devenv.fish:35 still invokes cmd_install $packages; in shells loaded from these dotfiles, running devenv now hits fish's unknown-command path instead of installing packages. Keep an alias/function or update that call to cmd-install.
Useful? React with 👍 / 👎.
0513f1d to
acc4a02
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acc4a02ea3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -25,18 +24,15 @@ return { | |||
| }, | |||
There was a problem hiding this comment.
When editing Markdown, this callback is ignored because blink.cmp validates the top-level option as enabled, not enable (see its v1 config source: https://github.com/Saghen/blink.cmp/blob/v1.10.2/lua/blink/cmp/config/init.lua). As a result, completion remains enabled in Markdown buffers and blink reports enable as an unexpected config field instead of applying this disable rule.
Useful? React with 👍 / 👎.
- Add jj config. I basically always use it instead of git, so I should have some config for it. - Add `.jj` directory to global git ignore - Add `.jj` to auto_root - Fix cmd_install fish by using python3. Not all os alias python to python3 - Add ~/.local/bin/ to path - Increase ghostty scrollback - Disable tab inherit directory in ghostty - Remove copilot, since I don't use it anymore - Disable cmp on Markdown files, it was getting pretty annoying. Maybe I should do that for git commit too - Use nvim_notify in db_ui
- Move config to be in .config by placing in xdg_config - Move the script to a bin directory and give it a shbang and make it executable - Delete the fish cmd_install and rely on this instead - Update the cmd_install completion to get the commands from the config file
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b18611c7c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| Python port of `xdg_config/fish/functions/cmd_install.fish`. | ||
| """ | ||
| #!/usr/bin/env python3 |
There was a problem hiding this comment.
When devenv or a direct cmd-install call runs on a host where python3 is still 3.10 or 3.11, this new shebang bypasses the deleted uv script metadata that required a modern interpreter: Python 3.10 aborts on tomllib, and Python 3.11 fails every archive install at tempfile.TemporaryDirectory(delete=False). Since the removed fish function did not depend on Python, keep the uv runner/metadata or otherwise gate the executable to a supported Python before routing users through it.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@bin/cmd-install`:
- Around line 843-850: Move the empty-command no-op check in cmd-install before
load_config() so the tool exits cleanly when args.commands is empty even if no
default config exists. Update the main command flow in the cmd-install
entrypoint to handle the “no positional commands” case first, then only call
load_config() and proceed with commands_cfg when there is at least one command
to install.
In `@setup`:
- Around line 16-17: Make the symlink creation in the setup script idempotent:
the current `ls bin | xargs ... ln -s` flow will fail on reruns when a target in
`$HOME/.local/bin` already exists. Update the bin-linking step to use a safe
replace-or-skip behavior so rerunning the script does not abort under `set -e`,
and keep the fix localized to the symlink creation logic.
In `@xdg_config/fish/functions/devenv.fish`:
- Line 35: The devenv launcher currently calls cmd-install directly, which
breaks existing installs when the new binary has not been bootstrapped yet.
Update the fish function devenv to preserve compatibility by falling back to the
legacy cmd_install path or otherwise ensuring the installer entrypoint is
available before invoking it, so existing users can still run devenv after
pulling this change.
In `@xdg_config/nvim/lua/plugins/dadbod.lua`:
- Around line 14-16: Move the Dadbod UI global setup out of the plugin’s config
callback and into its init phase so it is applied before `vim-dadbod-ui` loads.
Update the `dadbod` plugin spec to set `vim.g.db_ui_use_nvim_notify` in `init`
instead of `config`, keeping the change localized to the
`vim.g.db_ui_use_nvim_notify` assignment.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6b437e9-5696-472e-af63-c9d001629602
📒 Files selected for processing (20)
.buildkite/pipeline.yml.github/workflows/ci.yml.gitignorebin/cmd-installjustfilesetuptests/test_cmd_install.pytests/test_verify_cmd_install_shas.pyxdg_config/cmd_install/config.tomlxdg_config/fish/completions/cmd-install.fishxdg_config/fish/completions/cmd_install.fishxdg_config/fish/config.fishxdg_config/fish/functions/cmd_install.fishxdg_config/fish/functions/devenv.fishxdg_config/ghostty/configxdg_config/git/gitignorexdg_config/jj/config.tomlxdg_config/nvim/lua/plugins/cmp.luaxdg_config/nvim/lua/plugins/dadbod.luaxdg_config/nvim/lua/plugins/mini.lua
💤 Files with no reviewable changes (2)
- xdg_config/fish/completions/cmd_install.fish
- xdg_config/fish/functions/cmd_install.fish
| if args.list: | ||
| for command in commands_cfg.keys(): | ||
| print(command) | ||
| return 0 | ||
|
|
||
| if not args.commands: | ||
| log_warning("no commands specified, doing nothing...") | ||
| return 0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check for an empty command list before loading the config.
Running cmd-install with no positional commands still executes load_config() first, so a missing default config exits with an error instead of reaching the new no-op path. That undermines the newly optional commands behavior on a fresh machine.
Proposed fix
def main() -> int:
args = parse_args()
+
+ if not args.commands and not args.list:
+ log_warning("no commands specified, doing nothing...")
+ return 0
try:
log_info(f"Loading config from {args.config}")
commands_cfg = load_config(args.config)
except ConfigError as exc:
log_error(f"Config error: {exc}")
return 1
if args.list:
for command in commands_cfg.keys():
print(command)
return 0
-
- if not args.commands:
- log_warning("no commands specified, doing nothing...")
- return 0🤖 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 `@bin/cmd-install` around lines 843 - 850, Move the empty-command no-op check
in cmd-install before load_config() so the tool exits cleanly when args.commands
is empty even if no default config exists. Update the main command flow in the
cmd-install entrypoint to handle the “no positional commands” case first, then
only call load_config() and proceed with commands_cfg when there is at least one
command to install.
| mkdir -p $HOME/.local/bin | ||
| ls bin | xargs -I+ ln -s "$HERE/bin/+" $HOME/.local/bin/ |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make the bin/ symlink step idempotent.
Line 17 will abort every rerun once any ~/.local/bin/* link already exists, because ln -s returns EEXIST and set -e stops the script before the remaining setup runs.
Suggested fix
-mkdir -p $HOME/.local/bin
-ls bin | xargs -I+ ln -s "$HERE/bin/+" $HOME/.local/bin/
+mkdir -p "$HOME/.local/bin"
+for file in "$HERE"/bin/*; do
+ ln -sfn "$file" "$HOME/.local/bin/$(basename "$file")"
+done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mkdir -p $HOME/.local/bin | |
| ls bin | xargs -I+ ln -s "$HERE/bin/+" $HOME/.local/bin/ | |
| mkdir -p "$HOME/.local/bin" | |
| for file in "$HERE"/bin/*; do | |
| ln -sfn "$file" "$HOME/.local/bin/$(basename "$file")" | |
| done |
🤖 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 `@setup` around lines 16 - 17, Make the symlink creation in the setup script
idempotent: the current `ls bin | xargs ... ln -s` flow will fail on reruns when
a target in `$HOME/.local/bin` already exists. Update the bin-linking step to
use a safe replace-or-skip behavior so rerunning the script does not abort under
`set -e`, and keep the fix localized to the symlink creation logic.
|
|
||
| printf "Installing packages for the %s$env%s environment (%s$packages%s)\n" (set_color green) (set_color normal) (set_color yellow) (set_color normal) | ||
| cmd_install $packages | ||
| cmd-install $packages |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve an upgrade path for existing installs.
Line 35 assumes cmd-install is already on PATH, but this PR also removes the old cmd_install function and only creates the replacement binary when setup is rerun. After pulling the change, devenv becomes command not found for anyone who has not re-bootstrapped yet.
🤖 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 `@xdg_config/fish/functions/devenv.fish` at line 35, The devenv launcher
currently calls cmd-install directly, which breaks existing installs when the
new binary has not been bootstrapped yet. Update the fish function devenv to
preserve compatibility by falling back to the legacy cmd_install path or
otherwise ensuring the installer entrypoint is available before invoking it, so
existing users can still run devenv after pulling this change.
| config = function() | ||
| vim.g.db_ui_use_nvim_notify = 1 | ||
| end, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n[File under review]\n'
cat -n xdg_config/nvim/lua/plugins/dadbod.lua
printf '\n[Search for db_ui_use_nvim_notify]\n'
rg -n "db_ui_use_nvim_notify|nvim_notify|db_ui_" xdg_config/nvim -S
printf '\n[Search for dadbod plugin spec patterns]\n'
rg -n "vim-dadbod-ui|dadbod" xdg_config/nvim/lua -SRepository: kdeal/cs-dotfiles
Length of output: 1510
🌐 Web query:
lazy.nvim init vs config plugin option timing vim-dadbod-ui db_ui_use_nvim_notify README
💡 Result:
In lazy.nvim, the init and config options serve different purposes based on the plugin's loading lifecycle [1][2]. The init function is executed during Neovim startup for every plugin entry, regardless of whether the plugin is lazy-loaded or when it actually loads [1][3][4]. It is primarily intended for setting global configuration variables (e.g., vim.g.*) that the plugin requires during its own initialization process [1][2]. The config function is executed only when the plugin is actually loaded [5][1][2]. If you use the opts option, lazy.nvim automatically handles the configuration by calling the plugin's setup function (typically require('module').setup(opts)), which is the recommended approach over manually writing a config function [1][2][6]. For vim-dadbod-ui, the setting g:db_ui_use_nvim_notify = 1 is a global variable that configures the plugin's notification behavior [7][8]. Because it is a global variable, it should be set within an init function to ensure it is defined before the plugin loads [9][10][4]. To integrate vim-dadbod-ui with nvim-notify, follow these steps: 1. Configure the global variable using init in the vim-dadbod-ui spec [9][10]. 2. Redirect Neovim's notification API to nvim-notify, typically in the nvim-notify plugin spec [8]. Example configuration: { 'kristijanhusak/vim-dadbod-ui', init = function -- Set global variable before the plugin loads vim.g.db_ui_use_nvim_notify = 1 end, }, { 'rcarriga/nvim-notify', config = function -- Redirect vim.notify to nvim-notify vim.notify = require('notify') end, } This separation ensures the global configuration is ready when vim-dadbod-ui initializes, while the notification override is applied correctly when the notify plugin loads [8][4].
Citations:
- 1: https://github.com/folke/lazy.nvim/blob/1ea3c408/doc/lazy.nvim.txt
- 2: https://lazy.folke.io/spec
- 3: https://github.com/folke/lazy.nvim/blob/96584866b9c5e998cbae300594d0ccfd0c464627/README.md
- 4: https://www.adamolsen.dev/article/2024-11-20/advanced-lazynvim
- 5: Order of plugin loading folke/lazy.nvim#1427
- 6: https://github.com/folke/lazy.nvim/blob/main/doc/lazy.nvim.txt
- 7: https://github.com/kristijanhusak/vim-dadbod-ui/blob/master/doc/dadbod-ui.txt
- 8: Using nvim-notify kristijanhusak/vim-dadbod-ui#191
- 9: https://github.com/kristijanhusak/vim-dadbod-ui/blob/master/README.md
- 10: https://github.com/kristijanhusak/vim-dadbod-ui?tab=readme-ov-file
Move this Dadbod UI global to init. vim.g.db_ui_use_nvim_notify needs to be set before vim-dadbod-ui loads; config runs too late here, so the plugin can miss the option.
Suggested change
- config = function()
+ init = function()
vim.g.db_ui_use_nvim_notify = 1
end,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| config = function() | |
| vim.g.db_ui_use_nvim_notify = 1 | |
| end, | |
| init = function() | |
| vim.g.db_ui_use_nvim_notify = 1 | |
| end, |
🤖 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 `@xdg_config/nvim/lua/plugins/dadbod.lua` around lines 14 - 16, Move the Dadbod
UI global setup out of the plugin’s config callback and into its init phase so
it is applied before `vim-dadbod-ui` loads. Update the `dadbod` plugin spec to
set `vim.g.db_ui_use_nvim_notify` in `init` instead of `config`, keeping the
change localized to the `vim.g.db_ui_use_nvim_notify` assignment.
1 participant
Summary by CodeRabbit
New Features
cmd-installcommand with a--listoption, improved default configuration handling, and clearer status output.Bug Fixes