feat: improve Hyprland keybindings, add /bin shell symlinks, fix clipboard - #799
Conversation
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
Summary of ChangesHello @shunkakinoki, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the Hyprland desktop environment configuration by optimizing keybindings for window management and the lock screen, and by streamlining the clipboard management setup. It also addresses system compatibility by introducing necessary shell symlinks for NixOS. These changes aim to improve user experience, resolve keybinding conflicts, and ensure proper application behavior. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughConfiguration edits: Hyprland keybinding and clipboard cleanup, replacement of movewindow with movetoworkspace mappings, lock-screen modifier changed, removal of clipse TUI rules; added a new Linux-only home-manager activation module to create /bin symlinks for shells; xremap remap list excludes "l". Changes
Sequence Diagram(s)sequenceDiagram
participant HM as Home-Manager activation
participant SUDO as Sudo wrapper (/run/wrappers/bin/sudo or /usr/bin/sudo)
participant FS as Filesystem (/bin and symlinks)
participant NIX as Nix store binaries
HM->>SUDO: probe for sudo wrapper (multiple paths)
alt sudo available
HM->>SUDO: run_root_cmd wraps commands with sudo
else running as root or dry-run set
HM->>HM: run commands directly or via DRY_RUN_CMD
end
HM->>FS: create /bin directory
HM->>NIX: resolve shell binaries paths
HM->>FS: create symlinks `/bin/bash`, `/bin/fish`, `/bin/zsh` -> NIX binaries (via run_root_cmd)
FS-->>HM: symlink creation result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to improve Hyprland configuration with better keybindings, fix fullscreen behavior for Chrome, switch to cliphist for clipboard management, and enhance script compatibility on NixOS by adding shell symlinks in /bin. However, the introduction of the bin-shells module using sudo in Home Manager activation scripts poses a significant security risk, creating a privilege escalation vector and bypassing NixOS's declarative system configuration. It is strongly recommended to address this vulnerability by managing /bin symlinks at the system level.
| /run/wrappers/bin/sudo mkdir -p /bin | ||
| /run/wrappers/bin/sudo ln -sf ${pkgs.bash}/bin/bash /bin/bash | ||
| /run/wrappers/bin/sudo ln -sf ${pkgs.fish}/bin/fish /bin/fish | ||
| /run/wrappers/bin/sudo ln -sf ${pkgs.zsh}/bin/zsh /bin/zsh |
There was a problem hiding this comment.
The use of sudo in Home Manager activation scripts for managing /bin symlinks is a critical security vulnerability. This approach introduces a privilege escalation vector, as a compromised Nix configuration could lead to malicious binaries being installed in /bin, potentially resulting in a full system compromise. It also violates the principle of least privilege and bypasses the declarative nature of NixOS system configuration. System-wide symlinks should be managed declaratively in the system configuration (e.g., configuration.nix) using native options like environment.binsh, services.envfs, or system.activationScripts. While grouping sudo commands could improve efficiency, the fundamental issue is the use of sudo in this context, which should be avoided.
/run/wrappers/bin/sudo sh -c "mkdir -p /bin && ln -sf ${pkgs.bash}/bin/bash /bin/bash && ln -sf ${pkgs.fish}/bin/fish /bin/fish && ln -sf ${pkgs.zsh}/bin/zsh /bin/zsh"
![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
🤖 Fix all issues with AI agents
In `@home-manager/modules/bin-shells/default.nix`:
- Around line 7-12: The activation uses /run/wrappers/bin/sudo (in
home.activation.binShells) which can prompt or be missing on non-NixOS systems;
change the activation to run only on NixOS or avoid sudo: wrap
home.activation.binShells with a conditional check (e.g., if
config.system.build?.isNixos or lib.isNixos equivalent in your codebase) so the
mkdir/ln commands only run on NixOS, or remove sudo and instead implement these
links as a system-level module (environment.etc or system.activationScripts) if
you need them globally; update references to home.activation.binShells and
lib.hm.dag.entryAfter accordingly.
🧹 Nitpick comments (1)
config/hyprland/hyprland.conf (1)
279-283: H/L and K/J now perform different operations under the same "Move windows" heading.H/L use
movetoworkspace(cross-workspace) while K/J usemovewindow(within-workspace). Consider splitting these into two sub-sections or updating the comment to clarify the distinction, so the next reader doesn't assume all four keys do the same type of operation.📝 Suggested comment clarification
-# Move windows (vim keys) -bind = $mod SHIFT, H, movetoworkspace, r-1 -bind = $mod SHIFT, L, movetoworkspace, r+1 -bind = $mod SHIFT, K, movewindow, u -bind = $mod SHIFT, J, movewindow, d +# Move window to adjacent workspace (vim H/L) +bind = $mod SHIFT, H, movetoworkspace, r-1 +bind = $mod SHIFT, L, movetoworkspace, r+1 + +# Move window within workspace (vim K/J) +bind = $mod SHIFT, K, movewindow, u +bind = $mod SHIFT, J, movewindow, d
| home.activation.binShells = lib.hm.dag.entryAfter [ "writeBoundary" ] '' | ||
| /run/wrappers/bin/sudo mkdir -p /bin | ||
| /run/wrappers/bin/sudo ln -sf ${pkgs.bash}/bin/bash /bin/bash | ||
| /run/wrappers/bin/sudo ln -sf ${pkgs.fish}/bin/fish /bin/fish | ||
| /run/wrappers/bin/sudo ln -sf ${pkgs.zsh}/bin/zsh /bin/zsh | ||
| ''; |
There was a problem hiding this comment.
sudo in home-manager activation may prompt interactively or fail in non-interactive contexts.
This activation runs sudo on every home-manager switch, which could prompt for a password or fail in automated/CI contexts. Also, /run/wrappers/bin/sudo is NixOS-specific — on other Linux distros this path won't exist despite isLinux being true.
If this repo only targets NixOS, this is fine in practice. Otherwise, consider guarding with a NixOS-specific check or using a NixOS system-level module (environment.etc or system.activationScripts) which wouldn't need sudo from user space.
🤖 Prompt for AI Agents
In `@home-manager/modules/bin-shells/default.nix` around lines 7 - 12, The
activation uses /run/wrappers/bin/sudo (in home.activation.binShells) which can
prompt or be missing on non-NixOS systems; change the activation to run only on
NixOS or avoid sudo: wrap home.activation.binShells with a conditional check
(e.g., if config.system.build?.isNixos or lib.isNixos equivalent in your
codebase) so the mkdir/ln commands only run on NixOS, or remove sudo and instead
implement these links as a system-level module (environment.etc or
system.activationScripts) if you need them globally; update references to
home.activation.binShells and lib.hm.dag.entryAfter accordingly.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="home-manager/modules/bin-shells/default.nix">
<violation number="1" location="home-manager/modules/bin-shells/default.nix:8">
P2: The activation script hardcodes `/run/wrappers/bin/sudo` and runs commands directly, which fails on non-NixOS Linux and bypasses home-manager’s dry-run behavior. Resolve the sudo command dynamically and wrap calls with `$DRY_RUN_CMD` (as done in other modules).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
This PR updates the Linux desktop (Hyprland + xremap) configuration to refine keybindings, simplify clipboard management, and improve script compatibility by adding /bin/* shell symlinks.
Changes:
- Adjust Hyprland keybindings (cross-workspace window movement, lock-screen binding, fullscreen behavior).
- Remove
clipseusage from Hyprland config in favor ofcliphist+wl-clip-persist. - Add a new home-manager module that creates
/bin/{bash,fish,zsh}symlinks during activation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| home-manager/modules/xremap/default.nix | Excludes l from remapping so Hyper+L can reach Hyprland for locking. |
| home-manager/modules/default.nix | Registers the new bin-shells module. |
| home-manager/modules/bin-shells/default.nix | Adds activation logic intended to create /bin shell symlinks. |
| config/hyprland/hyprland.conf | Updates binds, removes clipse rules/binds, and adjusts clipboard + fullscreen behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /run/wrappers/bin/sudo mkdir -p /bin | ||
| /run/wrappers/bin/sudo ln -sf ${pkgs.bash}/bin/bash /bin/bash | ||
| /run/wrappers/bin/sudo ln -sf ${pkgs.fish}/bin/fish /bin/fish | ||
| /run/wrappers/bin/sudo ln -sf ${pkgs.zsh}/bin/zsh /bin/zsh |
There was a problem hiding this comment.
home.activation.binShells hard-codes /run/wrappers/bin/sudo and writes to /bin. This will fail for this repo’s homeConfigurations on generic Linux (e.g. Ubuntu) and for users without that NixOS wrapper path. Consider gating this to NixOS-only, and/or resolving an elevated command dynamically (sudo/doas/root) like the existing root-helper pattern in home-manager/modules/tailscale/default.nix, while also respecting $DRY_RUN_CMD to avoid mutating the system during dry runs.
| /run/wrappers/bin/sudo mkdir -p /bin | |
| /run/wrappers/bin/sudo ln -sf ${pkgs.bash}/bin/bash /bin/bash | |
| /run/wrappers/bin/sudo ln -sf ${pkgs.fish}/bin/fish /bin/fish | |
| /run/wrappers/bin/sudo ln -sf ${pkgs.zsh}/bin/zsh /bin/zsh | |
| ROOT_CMD="''${DRY_RUN_CMD:-}" | |
| # If no dry-run command is set, choose an appropriate root helper. | |
| if [ -z "$ROOT_CMD" ]; then | |
| if [ "$(id -u)" -eq 0 ]; then | |
| ROOT_CMD="" | |
| elif command -v sudo >/dev/null 2>&1; then | |
| ROOT_CMD="sudo" | |
| elif command -v doas >/dev/null 2>&1; then | |
| ROOT_CMD="doas" | |
| else | |
| echo "binShells activation: need sudo or doas (or run as root) to manage /bin shell links" >&2 | |
| exit 1 | |
| fi | |
| fi | |
| ''${ROOT_CMD} mkdir -p /bin | |
| ''${ROOT_CMD} ln -sf ${pkgs.bash}/bin/bash /bin/bash | |
| ''${ROOT_CMD} ln -sf ${pkgs.fish}/bin/fish /bin/fish | |
| ''${ROOT_CMD} ln -sf ${pkgs.zsh}/bin/zsh /bin/zsh |
| { lib, pkgs, ... }: | ||
| let | ||
| inherit (pkgs.stdenv) isLinux; | ||
| in | ||
| { | ||
| config = lib.mkIf isLinux { | ||
| home.activation.binShells = lib.hm.dag.entryAfter [ "writeBoundary" ] '' |
There was a problem hiding this comment.
This activation entry uses lib.hm.dag.entryAfter, but the other home-manager modules in this repo consistently use config.lib.dag.entryAfter. Aligning with that pattern avoids surprises and keeps the module interface consistent (also add config to the arg set if you switch).
| { lib, pkgs, ... }: | |
| let | |
| inherit (pkgs.stdenv) isLinux; | |
| in | |
| { | |
| config = lib.mkIf isLinux { | |
| home.activation.binShells = lib.hm.dag.entryAfter [ "writeBoundary" ] '' | |
| { config, lib, pkgs, ... }: | |
| let | |
| inherit (pkgs.stdenv) isLinux; | |
| in | |
| { | |
| config = lib.mkIf isLinux { | |
| home.activation.binShells = config.lib.dag.entryAfter [ "writeBoundary" ] '' |
Mesa DescriptionTL;DRImproved Hyprland keybindings and window management, fixed fullscreen overlap and clipboard persistence, and added What changed?
Description generated by Mesa. Update settings |
…board
- Change movewindow h/l to movetoworkspace r-1/r+1 for cross-workspace movement
- Move lock screen from Super+Shift+L to Framework+L to avoid conflict
- Exclude "l" from xremap so Framework+L passes through as Hyper
- Switch fullscreen toggle to maximize mode (fullscreen 1) to fix Chrome panel overlap
- Remove clipse clipboard manager in favor of cliphist only
- Add /bin/{bash,fish,zsh} symlinks via home-manager activation for script compatibility
a36321a to
e3ef43d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@home-manager/modules/bin-shells/default.nix`:
- Around line 15-17: The current activation step aborts the entire home-manager
activation by calling exit 1 in the branch that checks elif [ "$(id -u)" -ne 0
]; instead log the warning ("Creating /bin shell symlinks requires root
privileges, but sudo is not available.") to stderr and replace exit 1 with a
return so only this activation entry is skipped; update the conditional branch
that performs the /bin symlink creation to return after logging when not root
and sudo is unavailable.
| elif [ "$(id -u)" -ne 0 ]; then | ||
| echo "Creating /bin shell symlinks requires root privileges, but sudo is not available." >&2 | ||
| exit 1 |
There was a problem hiding this comment.
exit 1 aborts the entire home-manager activation, not just this step.
If sudo isn't available and the user isn't root, exit 1 will halt all remaining activation entries (config writes, service restarts, etc.). For a non-critical convenience feature like /bin symlinks, prefer logging a warning and continuing.
Proposed fix
elif [ "$(id -u)" -ne 0 ]; then
echo "Creating /bin shell symlinks requires root privileges, but sudo is not available." >&2
- exit 1
+ return 0
fiNote: home-manager activation entries are wrapped in functions, so return is valid here and will skip only this activation step.
📝 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.
| elif [ "$(id -u)" -ne 0 ]; then | |
| echo "Creating /bin shell symlinks requires root privileges, but sudo is not available." >&2 | |
| exit 1 | |
| elif [ "$(id -u)" -ne 0 ]; then | |
| echo "Creating /bin shell symlinks requires root privileges, but sudo is not available." >&2 | |
| return 0 |
🤖 Prompt for AI Agents
In `@home-manager/modules/bin-shells/default.nix` around lines 15 - 17, The
current activation step aborts the entire home-manager activation by calling
exit 1 in the branch that checks elif [ "$(id -u)" -ne 0 ]; instead log the
warning ("Creating /bin shell symlinks requires root privileges, but sudo is not
available.") to stderr and replace exit 1 with a return so only this activation
entry is skipped; update the conditional branch that performs the /bin symlink
creation to return after logging when not root and sudo is unavailable.
Summary
movewindowh/l tomovetoworkspacer-1/r+1 for cross-workspace window movementSuper+Shift+LtoFramework+Lto resolve conflict with workspace movementlfrom xremap soFramework+Lpasses through as Hyper to Hyprlandfullscreen 1) to fix Chrome overlapping hyprpanel/bin/{bash,fish,zsh}symlinks via home-manager activation for script compatibility on NixOSTest plan
Super+Shift+H/Lmoves windows between workspacesFramework+Llocks screen via hyprlockSuper+Ctrl+Fmaximizes without overlapping panelSuper+Shift+Vopens cliphist clipboard history/bin/bash,/bin/fish,/bin/zshsymlinks exist after switch🤖 Generated with Claude Code
Summary by cubic
Improved Hyprland window movement and lock screen shortcut, fixed fullscreen overlap and clipboard persistence, and added /bin shell symlinks with portable sudo detection for Linux.
New Features
Bug Fixes
Written for commit 8578c8a. Summary will update on new commits.