feat(noctalia): lid lock, AC-aware idle, 5min dimlock 10min suspend - #1651
Conversation
|
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
📝 WalkthroughWalkthroughA new Bash script checks AC power status and runs ChangesAC Power Idle Inhibition & Noctalia Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Mesa DescriptionTL;DRLock the session on lid close and make idle power-aware. On AC, idle is inhibited; on battery, the display turns off and locks at 5 minutes and the system suspends at 10 minutes. What changed?
Description generated by Mesa. Update settings |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/noctalia/ac-idle-inhibit.sh`:
- Around line 6-11: The loop in ac-idle-inhibit.sh refreshes systemd-inhibit
only every 20s, causing a stale inhibitor around idle deadlines; replace the
fixed sleep-based polling with an event-driven wait on the AC sysfs file (the
"$AC" variable) or a shorter reactive mechanism so the inhibitor is
created/removed immediately when AC state changes: stop relying on the 20s sleep
in the while-true loop, watch "$AC" for changes (e.g., inotify or udev/dbus) and
run or kill the systemd-inhibit invocation accordingly (reference the while loop
and the systemd-inhibit call in the script).
🪄 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: db98353d-87f8-4139-a76e-f818e3459666
📒 Files selected for processing (2)
config/noctalia/ac-idle-inhibit.shconfig/noctalia/default.nix
![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 introduces a mechanism to inhibit system idle when connected to AC power through a new bash script and a corresponding systemd user service. Additionally, it updates the desktop environment configuration, including UI adjustments (widgets, fonts, and dock indicators), security enhancements like locking on suspend, and specific idle timeouts for battery usage. Feedback focuses on improving the robustness of the AC detection script by avoiding hardcoded hardware paths, ensuring the systemd service explicitly includes necessary binaries in its environment, and optimizing the dark mode hook for better maintainability.
| AC=/sys/class/power_supply/ACAD/online | ||
|
|
||
| while true; do | ||
| if [ "$(cat "$AC" 2>/dev/null)" = "1" ]; then | ||
| systemd-inhibit --what=idle --why="On AC power" --mode=block sleep 20 | ||
| else | ||
| sleep 20 | ||
| fi | ||
| done |
There was a problem hiding this comment.
The script hardcodes the AC adapter path to /sys/class/power_supply/ACAD/online, which is hardware-dependent and may not exist on all systems (e.g., some use AC or ADP1). Using a glob with grep is more robust and avoids potential failures if the specific path is missing. Additionally, since set -e is used, the script would terminate if the hardcoded path was missing and accessed via a subshell in a different context.
| AC=/sys/class/power_supply/ACAD/online | |
| while true; do | |
| if [ "$(cat "$AC" 2>/dev/null)" = "1" ]; then | |
| systemd-inhibit --what=idle --why="On AC power" --mode=block sleep 20 | |
| else | |
| sleep 20 | |
| fi | |
| done | |
| while true; | |
| do | |
| if grep -q 1 /sys/class/power_supply/*/online 2>/dev/null; then | |
| systemd-inhibit --what=idle --why="On AC power" --mode=block sleep 20 | |
| else | |
| sleep 20 | |
| fi | |
| done |
| Service = { | ||
| Type = "simple"; | ||
| ExecStart = "${pkgs.bash}/bin/bash ${./ac-idle-inhibit.sh}"; | ||
| Restart = "on-failure"; | ||
| }; |
There was a problem hiding this comment.
The systemd service relies on systemd-inhibit, grep, and sleep being in the PATH. In a systemd user service, the environment might be restricted. It is safer to explicitly provide the required packages in the service's path attribute to ensure reliability across different environments.
path = [ pkgs.systemd pkgs.coreutils pkgs.gnugrep ];
Service = {
Type = "simple";
ExecStart = "${pkgs.bash}/bin/bash ${./ac-idle-inhibit.sh}";
Restart = "on-failure";
};
| darkModeChange = '' | ||
| if [ "$1" = "true" ]; then | ||
| dconf write /org/gnome/desktop/interface/color-scheme "'prefer-dark'" | ||
| dconf write /org/gnome/desktop/interface/gtk-theme "'Adwaita-dark'" | ||
| dconf write /org/gnome/desktop/interface/icon-theme "'Adwaita'" | ||
| else | ||
| dconf write /org/gnome/desktop/interface/color-scheme "'prefer-light'" | ||
| dconf write /org/gnome/desktop/interface/gtk-theme "'Adwaita'" | ||
| dconf write /org/gnome/desktop/interface/icon-theme "'Adwaita'" | ||
| fi | ||
| ''; |
There was a problem hiding this comment.
The icon-theme is set to the same value ('Adwaita') in both branches of the if statement. Moving it outside the conditional block improves maintainability. Additionally, using the absolute path to the dconf binary from pkgs makes the hook more robust against environment variations, adhering to the repository's pattern for Nix-extracted scripts.
darkModeChange = ''
${pkgs.dconf}/bin/dconf write /org/gnome/desktop/interface/icon-theme "'Adwaita'"
if [ "$1" = "true" ]; then
${pkgs.dconf}/bin/dconf write /org/gnome/desktop/interface/color-scheme "'prefer-dark'"
${pkgs.dconf}/bin/dconf write /org/gnome/desktop/interface/gtk-theme "'Adwaita-dark'"
else
${pkgs.dconf}/bin/dconf write /org/gnome/desktop/interface/color-scheme "'prefer-light'"
${pkgs.dconf}/bin/dconf write /org/gnome/desktop/interface/gtk-theme "'Adwaita'"
fi
'';
References
- Maintain consistency with established patterns for writing scripts that are extracted from Nix expressions.
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| capsuleOpacity = 0; | ||
| widgets.left = [ | ||
| { id = "Launcher"; } | ||
| { id = "Workspaces"; } |
There was a problem hiding this comment.
Invalid widget id — Workspaces (plural) is not registered in noctalia.
noctalia-shell @ 9f8dd48 only registers the singular Workspace in Services/UI/BarWidgetRegistry.qml, and BarWidgetLoader.qml loads widgets via _barWidgetsDir + widgetId + ".qml" against Modules/Bar/Widgets/Workspace.qml. With Workspaces (plural), BarWidgetRegistry.hasWidget() returns false, the loader logs "Widget not found in registry", and the slot renders nothing — so the workspaces indicator never appears between Launcher and Clock.
Fix: use the singular id.
| { id = "Workspaces"; } | |
| { id = "Workspace"; } |
|
|
||
| while true; do | ||
| if [ "$(cat "$AC" 2>/dev/null)" = "1" ]; then | ||
| systemd-inhibit --what=idle --why="On AC power" --mode=block sleep 20 |
There was a problem hiding this comment.
This systemd-inhibit --what=idle has no effect on noctalia or Hyprland.
noctalia-shell @ 9f8dd48 detects idle solely through Quickshell.Wayland.IdleMonitor → ext-idle-notify-v1 (Services/Power/IdleService.qml); a repo-wide grep for login1/logind/ListInhibitors in noctalia returns zero hits, and Services/Power/IdleInhibitorService.qml only emits outgoing inhibitors — it does not listen for external ones.
Hyprland is the same story: src/protocols/IdleNotify.cpp::CExtIdleNotification::update gates only on PROTO::idle->isInhibited, and src/managers/input/IdleInhibitor.cpp::CInputManager::recheckIdleInhibitorStatus only flips that flag for live zwp_idle_inhibit_manager_v1 surfaces or Hyprland idleinhibit window rules. There is no logind/sd-bus integration anywhere in the Hyprland tree.
Net effect with idle = { screenOffTimeout = 300; lockTimeout = 300; suspendTimeout = 600; }: the screen still goes off and locks at 5 min and the machine still suspends at 10 min while on AC. The unit + script run continuously but change nothing observable.
To actually keep the machine awake on AC, hold a Wayland surface inhibitor (e.g. wayland-idle-inhibitor/wlinhibit) or set a Hyprland idleinhibit always window rule — those are the only inputs to recheckIdleInhibitorStatus().
| Service = { | ||
| Type = "simple"; | ||
| ExecStart = "${pkgs.bash}/bin/bash ${./ac-idle-inhibit.sh}"; | ||
| Restart = "on-failure"; |
There was a problem hiding this comment.
Add RestartSec to avoid a tight restart loop on transient failures.
The script runs under set -euo pipefail, so any non-zero exit from systemd-inhibit (e.g., logind not yet reachable early in the graphical session, polkit denial, missing binary on PATH) aborts the loop. With Restart = "on-failure" and no RestartSec, systemd will respawn at the default ~100 ms until the default start-limit trips and the unit is marked failed silently.
Other similar units in this repo set this:
home-manager/services/keyd-application-mapper/default.nix:40—RestartSec = 3;named-hosts/matic/default.nix:462(wallpaper-power-monitor) —RestartSec = 5;
Suggested:
Restart = "on-failure";
RestartSec = 5;
| idle.enabled = true; | ||
| idle = { | ||
| enabled = true; | ||
| screenOffTimeout = 300; # 5 min on battery |
There was a problem hiding this comment.
Comment is misleading — these timeouts apply on AC too.
noctalia has no power-state awareness: Services/Power/IdleService.qml binds screenOffTimeout/lockTimeout/suspendTimeout directly to Quickshell.Wayland.IdleMonitor.timeout for all stages and never branches on AC vs battery. The # 5 min on battery / # 10 min on battery annotations imply per-power-state behavior that does not exist (and the ac-idle-inhibit unit does not change this — see the inline comment on ac-idle-inhibit.sh).
Drop the on battery qualifier or remove the comments.
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
|
|
||
| AC=/sys/class/power_supply/ACAD/online |
There was a problem hiding this comment.
Hardcoded ACAD path diverges from the repo's portable pattern.
This repo already has two AC-aware scripts that don't hardcode the adapter name:
home-manager/modules/local-scripts/decafinate.sh:42-46and:174-178iteratefor ac_path in /sys/class/power_supply/AC*/online; do ....scripts/wallpaper-power-check.shtakes the path as@ac_supply_path@viapkgs.replaceVars.
With the hardcoded path here, cat "$AC" 2>/dev/null returns empty when the file is missing, the test fails, and the script silently behaves as if on battery — with no warning. It works today only because config/noctalia/ is loaded under isDesktop = true, which only named-hosts/matic enables. If noctalia is ever turned on for another desktop host with a different adapter name (e.g. AC0, ADP1), this script will silently do nothing and the failure will be invisible.
Consider switching to the AC*/online glob to match the existing pattern.
![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 3 files (changes from recent commits).
Prompt for AI agents (unresolved 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="spec/coverage_spec.sh">
<violation number="1" location="spec/coverage_spec.sh:363">
P2: This new shell script is added to the coverage list without a corresponding spec-file check, so the coverage guard no longer verifies test-file presence for it.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| config/hyprland/scripts/record-screen.sh | ||
| config/hyprland/scripts/toggle-terminal.sh | ||
| config/k3s/activate.sh | ||
| config/noctalia/ac-idle-inhibit.sh |
There was a problem hiding this comment.
P2: This new shell script is added to the coverage list without a corresponding spec-file check, so the coverage guard no longer verifies test-file presence for it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/coverage_spec.sh, line 363:
<comment>This new shell script is added to the coverage list without a corresponding spec-file check, so the coverage guard no longer verifies test-file presence for it.</comment>
<file context>
@@ -360,6 +360,7 @@ config/git-ai/activate.sh
config/hyprland/scripts/record-screen.sh
config/hyprland/scripts/toggle-terminal.sh
config/k3s/activate.sh
+config/noctalia/ac-idle-inhibit.sh
config/obsidian/activate.sh
config/omp/activate.sh
</file context>
…1651) * chore: update noctalia * feat: lock screen on lid close * feat: ac-aware idle - inhibit on AC, 5min dimlock 10min suspend on battery * fix: extract ac-idle-inhibit to external script for inline check * fix: reduce AC poll to 2s, add coverage entry, fix nix formatting
1 participant
Summary
general.lockOnSuspend = true(noctalia callslockAndSuspend()instead ofsuspend())ac-idle-inhibit) that holds asystemd-inhibit --what=idlelock while/sys/class/power_supply/ACAD/onlinereads1screenOffTimeout/lockTimeout = 300), suspend at 10 min (suspendTimeout = 600)ac-idle-inhibit.shto satisfyshell-inline-checkTest plan
shell-inline-checkpassesshell-testpasses🤖 Generated with Claude Code
Summary by cubic
Lock the session on lid close and make idle power-aware. On AC, idle is inhibited; on battery, the display turns off and locks at 5 minutes and the system suspends at 10 minutes.
New Features
lockOnSuspendto lock on lid close.systemduser serviceac-idle-inhibitthat polls AC state every 2s and holdssystemd-inhibitwhen online.screenOffTimeout/lockTimeout= 300s,suspendTimeout= 600s.noctalia: add Workspaces widget, switch default font to Noto Sans, hide dock indicator, align GTK/icon themes with dark mode.Bug Fixes
ac-idle-inhibit.sh, add coverage entry, and fix Nix formatting.Written for commit ca98d82. Summary will update on new commits.