Skip to content

Comments

Dev#4

Merged
ClementBobin merged 71 commits intomainfrom
DEV
Jun 13, 2025
Merged

Dev#4
ClementBobin merged 71 commits intomainfrom
DEV

Conversation

@ClementBobin
Copy link
Owner

Description

This pull request introduces significant updates to the configuration and module structure of the repository, with a focus on modularization, new feature additions, and cleanup of deprecated configurations. Key changes include the integration of deploy-rs for deployment management, consolidation of CLI tools and documentation modules, and removal of unused or outdated modules.

Deployment Enhancements:

  • Added deploy-rs integration in flake.nix, including deployment configurations for nodes (fern.local and oak.local) and a helper script (rb) for streamlined deployment commands. [1] [2]

Modularization and Cleanup:

  • Consolidated CLI tools (vercel and graphite) into a single module dev/global-tools/cli.nix, replacing individual modules. [1] [2] [3]
  • Unified documentation-related modules (okular, onlyoffice) into a centralized documentation module. [1] [2] [3]
  • Removed unused or redundant modules, including multimedia/easyeffects and utilities/gitkraken. [1] [2]

New Features:

  • Introduced a browser configuration module to manage browser emulators and drivers for automation (e.g., Selenium).
  • Added a game engine module to manage installations like Unity Hub. [1] [2]

Configuration Adjustments:

  • Updated flake.nix to use the nyxpkgs-unstable branch for chaotic.url and added deploy-rs as an input. [1] [2]
  • Adjusted default values and removed unnecessary enable options in several modules (e.g., mail, disk-usage). [1] [2]

These changes improve the maintainability and scalability of the repository while introducing new capabilities for deployment and modular configuration.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Checklist

  • My commits follow conventional commit format
  • I have updated the documentation accordingly
  • My changes generate no new warnings

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

♻️ Duplicate comments (4)
modules/system/common/games/games.nix (1)

22-51: Nice! GameMode settings are now configurable 🎉

This fully addresses the earlier feedback about hard-coded GameMode settings and greatly improves downstream composability.

modules/system/common/nix/polkit.nix (1)

13-18: ⚠️ Potential issue

Polkit rule grants blanket disk power – restrict or document

All members of the users group receive full org.freedesktop.udisks2.* rights (format, wipe, etc.). This broad scope is a known security foot-gun.

Options:

  1. Restrict to a dedicated group (e.g. storage).
  2. Limit to the subset actually required (filesystem-mount*).
  3. If intentional, add a comment explaining the trade-off.
-        if (action.id.indexOf("org.freedesktop.udisks2.") == 0 &&
-            subject.isInGroup("users")) {
+        if (action.id.indexOf("org.freedesktop.udisks2.filesystem-mount") == 0 &&
+            subject.isInGroup("storage")) {
hosts/fern/default.nix (1)

73-73: ⚠️ Potential issue

system.stateVersion set to unreleased “25.05”

NixOS 25.05 is not released; evaluations will fail on stable channels.

-  system.stateVersion = "25.05";
+  # Use the latest released version your configuration supports.
+  system.stateVersion = "24.05";
modules/hm/common/engine/default.nix (1)

16-18: Unfree package guard & minor style touch-up

unityhub is unfree; evaluation will break unless nixpkgs.config.allowUnfree = true is set higher up.
Also, the outer parentheses are superfluous – drop them for readability.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 028fc8e and e1faf67.

📒 Files selected for processing (17)
  • flake.nix (3 hunks)
  • hosts/fern/default.nix (2 hunks)
  • hosts/oak/default.nix (1 hunks)
  • modules/hm/common/browser/default.nix (1 hunks)
  • modules/hm/common/communication/mail/default.nix (1 hunks)
  • modules/hm/common/dev/global-tools/cli.nix (1 hunks)
  • modules/hm/common/documentation/default.nix (1 hunks)
  • modules/hm/common/engine/default.nix (1 hunks)
  • modules/hm/common/utilities/kde-connect.nix (1 hunks)
  • modules/hm/desktops/default.nix (0 hunks)
  • modules/hm/hosts/oak/default.nix (4 hunks)
  • modules/system/common/games/games.nix (2 hunks)
  • modules/system/common/hardware/powersave.nix (1 hunks)
  • modules/system/common/networks/print/print.nix (1 hunks)
  • modules/system/common/nix/linux-cachyos.nix (2 hunks)
  • modules/system/common/nix/polkit.nix (1 hunks)
  • modules/system/common/security/password-manager.nix (1 hunks)
💤 Files with no reviewable changes (1)
  • modules/hm/desktops/default.nix
🧰 Additional context used
🪛 GitHub Actions: Flake Check
modules/hm/common/documentation/default.nix

[error] 17-17: Syntax error, unexpected CONCAT operator at line 17, column 72. Likely caused by incorrect use of '++' operator in Nix expression.

flake.nix

[error] 95-96: Error while checking flake output 'nixosConfigurations' at nixosConfigurations.fern. The NixOS configuration evaluation failed due to syntax error in dependent module.

🔇 Additional comments (9)
modules/system/common/networks/print/print.nix (1)

15-21: Good: driver list is now user-configurable

Exposing drivers solves the inflexibility flagged earlier – nice!

hosts/fern/default.nix (1)

45-45: Import path looks good

Switching to the concrete hydenix.nix file clarifies intent and avoids a redundant wrapper.

hosts/oak/default.nix (1)

60-64:

Details

✅ Verification successful

Validate the new Home-Manager desktop import

../../modules/hm/desktops/hydenix.nix replaces the former directory wrapper.
Please double-check that the file really exists on all branches – a missing path will make nixos-rebuild fail at evaluation time.


🏁 Script executed:

#!/usr/bin/env bash
# Confirm the target file is present exactly once in the repo
fd hydenix.nix | grep -F 'modules/hm/desktops/hydenix.nix'

Length of output: 90


Confirm Home-Manager desktop import path

The file modules/hm/desktops/hydenix.nix is present exactly once in the repository, so the import in hosts/oak/default.nix is valid.

modules/hm/common/communication/mail/default.nix (1)

21-23: Good catch reverting to an empty default

Restoring default = [] keeps mail clients opt-in and avoids surprise installs. 👍

modules/hm/hosts/oak/default.nix (2)

31-34: google-chrome / chromedriver require allowUnfree – double-check host-level nixpkgs config
Both google-chrome and chromedriver are unfree; if nixpkgs.config.allowUnfree = true; isn’t set for this host, evaluation will fail.


76-79: Alphabetisation nit resolved – looks good
kde-connect now precedes scalar; previous merge-conflict risk removed.

modules/system/common/security/password-manager.nix (1)

26-30: Nice – udev + YubiKey manager packages now covered
Adding yubikey-manager alongside yubikey-personalization closes the gap raised in the prior review.

flake.nix (1)

5-5: Unpinned input lowers reproducibility
Tracking the moving branch nyxpkgs-unstable invites non-deterministic builds. Pin to a commit or tag instead.

-chaotic.url = "github:chaotic-cx/nyx/nyxpkgs-unstable";
+chaotic.url = "github:chaotic-cx/nyx/nyxpkgs-unstable?rev=<commit-hash>";
modules/system/common/hardware/powersave.nix (1)

60-62: Verify amdctl derivation availability
amdctl isn’t in upstream nixpkgs; evaluation will fail unless you overlay it. Double-check the package exists or switch to ryzenadj/amdgpu-top.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

♻️ Duplicate comments (5)
modules/hm/common/utilities/kde-connect.nix (1)

9-16: Kebab-case option key complicates usage.

Using a hyphen in the option name (kde-connect) forces quoting when users configure it (modules.common.utilities."kde-connect".enable). Consider renaming to kdeConnect or kde_connect for a more idiomatic and user-friendly API.

modules/hm/common/browser/default.nix (1)

20-37: 🧹 Nitpick (assertive)

Unfree browsers & drivers may fail to build without allowUnfree

google-chrome, microsoft-edge, vivaldi, brave, and their drivers are all unfree. Unless the system already sets nixpkgs.config.allowUnfree = true, selecting any of those emulators will break evaluation.

Consider adding:

  config = {
+   nixpkgs.config.allowUnfree = true;
     home.packages = with pkgs;

or at least documenting the requirement in the option descriptions.

modules/system/common/nix/polkit.nix (1)

13-20: Polkit rule still overly broad – reconsider or document

Granting every member of the users group unconditional access to the full org.freedesktop.udisks2.* namespace (including formatting) remains a serious attack surface. Prior feedback already highlighted this; please narrow the rule (e.g. filesystem-mount*) or dedicate a smaller group, or add an explicit comment justifying the trade-off.

flake.nix (2)

5-5: Unpinned chaotic input reduces build reproducibility
Same concern as the earlier review: tracking the moving branch nyxpkgs-unstable means every rebuild can produce different results. Pin to a specific commit/tag or rely on the flake lock to keep it deterministic.


114-127: ⚠️ Potential issue

rb helper crashes with set -u when no arg provided

With set -u, $1 is unset when the script is invoked without arguments, so the script exits with “unbound variable” before your usage message executes. Initialise host defensively:

-  host=$1
+  host=${1:-}

Entire diff:

@@
-  set -euo pipefail
-  host=$1
+  set -euo pipefail
+  host=${1:-}
@@
     *) echo "Usage: rb [oak|fern|all]" >&2; exit 1 ;;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e1faf67 and 02558e0.

📒 Files selected for processing (10)
  • flake.nix (3 hunks)
  • modules/hm/common/browser/default.nix (1 hunks)
  • modules/hm/common/dev/global-tools/cli.nix (1 hunks)
  • modules/hm/common/documentation/default.nix (1 hunks)
  • modules/hm/common/utilities/kde-connect.nix (1 hunks)
  • modules/system/common/games/games.nix (2 hunks)
  • modules/system/common/networks/print/print.nix (1 hunks)
  • modules/system/common/nix/linux-cachyos.nix (2 hunks)
  • modules/system/common/nix/polkit.nix (1 hunks)
  • modules/system/common/security/password-manager.nix (1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Flake Check
flake.nix

[error] 95-96: Nix flake check failed: Configuration error in 'nixosConfigurations.fern'. Exactly one of users.users.richen.isSystemUser and users.users.richen.isNormalUser must be set.


[error] 95-96: Nix flake check failed: Configuration error in 'nixosConfigurations.fern'. users.users.richen.group is unset. This used to default to nogroup, but this is unsafe. Please set users.users.richen.group and define users.groups.richen.

🔇 Additional comments (11)
modules/hm/common/utilities/kde-connect.nix (3)

1-4: Remove unused arguments: Good cleanup.

The function signature now uses { config, lib, ... }, and the previously unused pkgs argument has been removed, silencing linter warnings.


5-8: Simplify config access with let-binding: Approved.

Assigning cfg via a let binding makes downstream references concise and maintainable.


18-21: Verify NixOS service attributes exist.

Please confirm that services.kdeconnect.enable and services.kdeconnect.indicator are valid NixOS options (the upstream service may be named kdeconnectd).

modules/hm/common/dev/global-tools/cli.nix (1)

17-20: Correct use of lib.optional resolves the previous list-of-lists issue

The switch from lib.optionals to lib.optional with ++ concatenation gives a flat list and prevents the evaluation error flagged in the previous review. Looks good.

modules/hm/common/documentation/default.nix (1)

16-19: Guard the unfree onlyoffice-bin derivation

onlyoffice-bin is marked unfree in nixpkgs; evaluation will fail unless nixpkgs.config.allowUnfree = true. Either:

  1. Enable it automatically when the user selects the editor, or
  2. Document the requirement in the option description.

Would you like a follow-up patch that sets nixpkgs.config.allowUnfree = lib.elem "onlyoffice" cfg.editor;?

modules/system/common/nix/polkit.nix (1)

1-2: 👍 Unused-arg warnings resolved

pkgs and inputs were dropped from the argument list; the previous linter warning is gone.

modules/system/common/nix/linux-cachyos.nix (1)

10-17: Nice: separate flag for SCX service

Splitting enableSCX out from the main switch avoids surprising side-effects—good improvement.

flake.nix (2)

72-75: Overlay wiring looks correctinputs.deploy-rs.overlays.default is attached to the imported nixpkgs.


131-138: checks block now correctly top-level – flake schema satisfied
Nice fix; this resolves the earlier validation error.

modules/system/common/networks/print/print.nix (1)

25-28: 🛠️ Refactor suggestion

cfg.drivers is still evaluated unconditionally

Because function arguments are evaluated before lib.mkIf is applied, the
expression cfg.drivers is realised even when cfg.cups.enable = false.
To avoid forcing evaluation of (possibly large/unfree) derivations, wrap the
entire drivers attribute instead:

-    services.printing.drivers = lib.mkIf cfg.cups.enable cfg.drivers;
+    services.printing = lib.mkIf cfg.cups.enable {
+      drivers = cfg.drivers;
+    };

This delays evaluation until CUPS is enabled.

Likely an incorrect or invalid review comment.

modules/system/common/games/games.nix (1)

64-74: 🧹 Nitpick (assertive)

Minor: avoid eager evaluation inside mkIf

As with the print module, the attrset passed to lib.mkIf is evaluated
beforehand, so cfg.gamemode.* is forced even when GameMode is disabled.
Not critical here but you can apply the same lazy pattern:

-      (lib.mkIf cfg.gamemode.enable {
-        gamemode = {
-          enable = true;
-          ...
-        };
-      })
+      (lib.mkIf cfg.gamemode.enable (
+        lib.mkForce {
+          gamemode = {
+            enable = true;
+            ...
+          };
+        }
+      ))

Keeps large attrsets totally out of the evaluation when not needed.

Likely an incorrect or invalid review comment.

@ClementBobin ClementBobin merged commit 6b5b983 into main Jun 13, 2025
1 of 3 checks passed
@github-actions
Copy link

🎉 This PR is included in version 1.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@coderabbitai coderabbitai bot mentioned this pull request Jun 15, 2025
Merged
7 tasks
@coderabbitai coderabbitai bot mentioned this pull request Sep 1, 2025
Merged
7 tasks
@coderabbitai coderabbitai bot mentioned this pull request Nov 13, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant