Dankman - #1272
Merged
Merged
Conversation
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
📝 WalkthroughWalkthroughThe PR introduces cross-platform local scripts for clipboard and notification handling, replaces hardcoded notification mechanisms with these scripts, adds Darkman dark/light mode integration with theme switching, creates custom Ghostty themes, updates all shell configurations to expose new scripts in PATH, and removes hardcoded GTK dark-mode preferences in favor of dynamic theme switching. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
Contributor
Summary of ChangesHello, 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 significantly enhances the system's adaptability by introducing a robust, automated dark/light theme switching mechanism via the Highlights
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. Footnotes
|
Mesa DescriptionTL;DRAdds cross-platform notification and clipboard helpers, integrates them into What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new darkman service for automatic theme switching on Linux, refactors clipboard and notification handling into shared local scripts, and updates Ghostty terminal themes. Review comments highlight issues with line continuations in the darkman service scripts, which cause dconf commands to fail, and suggest an improvement for the notify-local script to utilize the sound parameter with terminal-notifier for better consistency.
Comment on lines
+13
to
+16
| ${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'" |
Contributor
There was a problem hiding this comment.
The backslash \ for line continuation causes the two dconf commands to be concatenated into a single, invalid command. This will prevent the dark mode theme from being applied correctly. Each command should be on its own line without continuation.
${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'"
Comment on lines
+21
to
+24
| ${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'" |
Contributor
There was a problem hiding this comment.
Similar to the darkModeScripts, the backslash \ for line continuation will cause these two dconf commands to be incorrectly concatenated, leading to a script failure. Please remove the line continuation to ensure each command runs separately.
${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'"
Comment on lines
+82
to
+83
| terminal-notifier -title "$title" -message "$message" >/dev/null 2>&1 || true | ||
| exit 0 |
Contributor
There was a problem hiding this comment.
The notify-local script receives a sound parameter, but it's not being used when terminal-notifier is the backend. terminal-notifier supports sounds via the -sound flag. Using this would make notifications more consistent across different backends on macOS.
if [[ -n "$sound" ]]; then
terminal-notifier -title "$title" -message "$message" -sound "$sound" >/dev/null 2>&1 || true
else
terminal-notifier -title "$title" -message "$message" >/dev/null 2>&1 || true
fi
exit 0
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces Darkman-based light/dark theme switching and refactors local clipboard/notification helpers, along with Ghostty theme configuration updates.
Changes:
- Add a Home Manager
services.darkmanmodule and set the portal Settings backend to Darkman. - Add shared
~/.local/bin/clipboard-copyand~/.local/bin/notify-localhelpers and wire them into tmux/Codex/Claude. - Add custom Ghostty themes and switch Ghostty config to use dark/light theme mapping.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| named-hosts/matic/default.nix | Prefer Darkman for the portal Settings implementation. |
| home-manager/services/default.nix | Register new darkman Home Manager service module. |
| home-manager/services/darkman/default.nix | Add Darkman service config and scripts to toggle GTK theme via dconf. |
| home-manager/programs/tmux/tmux.conf | Switch tmux clipboard command to shared local helper script. |
| home-manager/modules/local-binaries/default.nix | Add clipboard-copy and notify-local local executables. |
| config/gtk/default.nix | Remove hard-coded dark preference and set base GTK theme to Adwaita. |
| config/ghostty/themes/Dracula Custom | Add custom Dracula theme file for Ghostty. |
| config/ghostty/themes/Catppuccin Latte Custom | Add custom Catppuccin Latte theme file for Ghostty. |
| config/ghostty/default.nix | Install the custom Ghostty theme files into XDG config. |
| config/ghostty/config | Use dark/light theme mapping and remove split styling now owned by themes. |
| config/codex/config.tpl.toml | Route notifications through notify-local. |
| config/codex/config.toml | Route notifications through notify-local. |
| config/claude/notify.sh | Route notifications through notify-local when available. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+11
to
+15
| xdg.configFile."ghostty/themes/Dracula Custom" = { | ||
| source = ./themes + "/Dracula Custom"; | ||
| }; | ||
| xdg.configFile."ghostty/themes/Catppuccin Latte Custom" = { | ||
| source = ./themes + "/Catppuccin Latte Custom"; |
There was a problem hiding this comment.
Using theme filenames with spaces and building the source path via string concatenation is fragile in Nix/Home Manager (spaces can cause awkward store name/path handling, and string-vs-path coercion can be surprising). Consider renaming the theme files to space-free names (e.g., Dracula-Custom, Catppuccin-Latte-Custom) and referencing them as proper Nix paths so source is unambiguously a path.
Suggested change
| xdg.configFile."ghostty/themes/Dracula Custom" = { | |
| source = ./themes + "/Dracula Custom"; | |
| }; | |
| xdg.configFile."ghostty/themes/Catppuccin Latte Custom" = { | |
| source = ./themes + "/Catppuccin Latte Custom"; | |
| xdg.configFile."ghostty/themes/Dracula-Custom" = { | |
| source = ./themes/Dracula-Custom; | |
| }; | |
| xdg.configFile."ghostty/themes/Catppuccin-Latte-Custom" = { | |
| source = ./themes/Catppuccin-Latte-Custom; |
Comment on lines
+11
to
+26
| darkModeScripts = { | ||
| gtk-theme = '' | ||
| ${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'" | ||
| ''; | ||
| }; | ||
| lightModeScripts = { | ||
| gtk-theme = '' | ||
| ${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'" | ||
| ''; | ||
| }; |
There was a problem hiding this comment.
These scripts only update GNOME dconf keys. However, Home Manager’s gtk.theme.name typically writes ~/.config/gtk-3.0/settings.ini / gtk-4.0/settings.ini (and similar), which many GTK apps read directly. That can lead to inconsistent theming where apps remain on the statically configured theme even after Darkman toggles dconf. To keep behavior consistent, either (a) have Darkman scripts also update the GTK settings.ini theme entries, or (b) avoid enforcing a static gtk.theme.name and rely on a single source of truth for theme switching.
Comment on lines
+21
to
+25
| if command -v xclip >/dev/null 2>&1; then | ||
| exec xclip -selection clipboard | ||
| fi | ||
|
|
||
| if command -v xsel >/dev/null 2>&1; then |
There was a problem hiding this comment.
For X11 backends, xclip/xsel typically require a valid X display. As written, this will attempt to exec them even when $DISPLAY is unset (e.g., SSH, headless shells), which will fail noisily and prevent the script from reaching the final error message. Consider gating these branches on $DISPLAY being set (similar to the Wayland check).
Suggested change
| if command -v xclip >/dev/null 2>&1; then | |
| exec xclip -selection clipboard | |
| fi | |
| if command -v xsel >/dev/null 2>&1; then | |
| if [[ -n "''${DISPLAY:-}" ]] && command -v xclip >/dev/null 2>&1; then | |
| exec xclip -selection clipboard | |
| fi | |
| if [[ -n "''${DISPLAY:-}" ]] && command -v xsel >/dev/null 2>&1; then |
| # Clipboard | ||
| set -g set-clipboard on | ||
| set -g @clipboard_cmd "command -v pbcopy >/dev/null 2>&1 && pbcopy || { [ -n \"$WAYLAND_DISPLAY\" ] && wl-copy || xclip -selection clipboard; }" | ||
| set -g @clipboard_cmd "~/.local/bin/clipboard-copy" |
There was a problem hiding this comment.
Using ~ in tmux option values can be brittle because expansion depends on how/where the value is later executed. Using an explicit $HOME/.local/bin/clipboard-copy (or an absolute path) is more robust across tmux/plugin execution contexts.
Suggested change
| set -g @clipboard_cmd "~/.local/bin/clipboard-copy" | |
| set -g @clipboard_cmd "$HOME/.local/bin/clipboard-copy" |
| inherit (pkgs) lib; | ||
| in | ||
| { | ||
| services.darkman = lib.mkIf pkgs.stdenv.isLinux { |
There was a problem hiding this comment.
The PR title suggests the scope is Darkman-focused, but the changes also introduce clipboard/notification helper binaries and Ghostty theme updates. Consider updating the PR title/description to reflect the broader scope or splitting the unrelated changes into separate PRs to make review and rollback safer.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
home-manager/services/darkman/default.nix (1)
11-25: Consider deduplicating repeated dconf paths/theme literals.Small constant extraction would reduce drift risk between dark/light scripts.
♻️ Optional refactor
{ pkgs, ... }: let inherit (pkgs) lib; + dconf = "${pkgs.dconf}/bin/dconf"; + iface = "/org/gnome/desktop/interface"; + darkTheme = "Adwaita-dark"; + lightTheme = "Adwaita"; in { services.darkman = lib.mkIf pkgs.stdenv.isLinux { @@ darkModeScripts = { gtk-theme = '' - ${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'" + ${dconf} write \ + ${iface}/color-scheme "'prefer-dark'" + ${dconf} write \ + ${iface}/gtk-theme "'${darkTheme}'" ''; }; lightModeScripts = { gtk-theme = '' - ${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'" + ${dconf} write \ + ${iface}/color-scheme "'prefer-light'" + ${dconf} write \ + ${iface}/gtk-theme "'${lightTheme}'" ''; }; }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/services/darkman/default.nix` around lines 11 - 25, Extract repeated literals into small constants and reference them from the darkModeScripts and lightModeScripts blocks: pull out the dconf binary path (${pkgs.dconf}/bin/dconf), the interface keys (/org/gnome/desktop/interface/color-scheme and /org/gnome/desktop/interface/gtk-theme), and the theme names ("Adwaita" / "Adwaita-dark") into local variables at the top of the file, then use those variables inside the gtk-theme script strings in darkModeScripts and lightModeScripts so the same identifiers (e.g., dconfBinary, colorSchemeKey, gtkThemeKey, adwaitaTheme, adwaitaDarkTheme) are reused instead of duplicating literals.
🤖 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/codex/config.toml`:
- Line 4: The hook currently calls "$HOME/.local/bin/notify-local" unguarded;
update the command string so the notifier is only invoked if present (e.g., test
-x "$HOME/.local/bin/notify-local" or command -v notify-local) before running
the notify step that uses JSON, LAST_MESSAGE and notify-local, otherwise skip
notification to avoid failing the hook when the binary is missing.
In `@home-manager/modules/local-binaries/default.nix`:
- Around line 62-84: The notifier blocks (commands osascript, notify-send,
terminal-notifier called in the script around the escape_applescript usage)
currently use "|| true" and then unconditionally exit, which masks failures and
prevents fallbacks; change each block to run the notifier command without
forcing success, capture its exit status, and only run exit 0 when the command
succeeded (e.g., test the command's return code or use && exit 0 after the
notifier invocation) so that if osascript/notify-send/terminal-notifier exists
but fails, the script will continue to try the next backend instead of silently
stopping.
---
Nitpick comments:
In `@home-manager/services/darkman/default.nix`:
- Around line 11-25: Extract repeated literals into small constants and
reference them from the darkModeScripts and lightModeScripts blocks: pull out
the dconf binary path (${pkgs.dconf}/bin/dconf), the interface keys
(/org/gnome/desktop/interface/color-scheme and
/org/gnome/desktop/interface/gtk-theme), and the theme names ("Adwaita" /
"Adwaita-dark") into local variables at the top of the file, then use those
variables inside the gtk-theme script strings in darkModeScripts and
lightModeScripts so the same identifiers (e.g., dconfBinary, colorSchemeKey,
gtkThemeKey, adwaitaTheme, adwaitaDarkTheme) are reused instead of duplicating
literals.
🪄 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: a7bf1185-19ed-42cf-9942-d9676cd3716e
📒 Files selected for processing (13)
config/claude/notify.shconfig/codex/config.tomlconfig/codex/config.tpl.tomlconfig/ghostty/configconfig/ghostty/default.nixconfig/ghostty/themes/Catppuccin Latte Customconfig/ghostty/themes/Dracula Customconfig/gtk/default.nixhome-manager/modules/local-binaries/default.nixhome-manager/programs/tmux/tmux.confhome-manager/services/darkman/default.nixhome-manager/services/default.nixnamed-hosts/matic/default.nix
Comment on lines
+62
to
+84
| if command -v osascript >/dev/null 2>&1; then | ||
| escaped_title="$(escape_applescript "$title")" | ||
| escaped_message="$(escape_applescript "$message")" | ||
|
|
||
| if [[ -n "$sound" ]]; then | ||
| escaped_sound="$(escape_applescript "$sound")" | ||
| osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\" sound name \"$escaped_sound\"" >/dev/null 2>&1 || true | ||
| else | ||
| osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\"" >/dev/null 2>&1 || true | ||
| fi | ||
|
|
||
| exit 0 | ||
| fi | ||
|
|
||
| if command -v notify-send >/dev/null 2>&1; then | ||
| notify-send "$title" "$message" >/dev/null 2>&1 || true | ||
| exit 0 | ||
| fi | ||
|
|
||
| if command -v terminal-notifier >/dev/null 2>&1; then | ||
| terminal-notifier -title "$title" -message "$message" >/dev/null 2>&1 || true | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file
find . -name "default.nix" -path "*/local-binaries/*" | head -20Repository: shunkakinoki/dotfiles
Length of output: 114
🏁 Script executed:
# Check if the file exists and read the relevant lines
if [ -f "home-manager/modules/local-binaries/default.nix" ]; then
wc -l "home-manager/modules/local-binaries/default.nix"
echo "=== Lines 62-84 ==="
sed -n '62,84p' "home-manager/modules/local-binaries/default.nix" | cat -n
fiRepository: shunkakinoki/dotfiles
Length of output: 1190
🏁 Script executed:
# Get more context to understand the full function
sed -n '50,90p' "home-manager/modules/local-binaries/default.nix" | cat -nRepository: shunkakinoki/dotfiles
Length of output: 1557
Prevent silent notification failures by only exiting when notifier commands succeed.
Currently, || true masks command failures on lines 68, 70, 77, and 82, but each conditional block still unconditionally exits (lines 73, 78, 83), preventing fallback to the next backend when a notifier fails. This can silently drop notifications even when alternative backends are available.
🔧 Suggested fix
if command -v osascript >/dev/null 2>&1; then
escaped_title="$(escape_applescript "$title")"
escaped_message="$(escape_applescript "$message")"
if [[ -n "$sound" ]]; then
escaped_sound="$(escape_applescript "$sound")"
- osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\" sound name \"$escaped_sound\"" >/dev/null 2>&1 || true
+ if osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\" sound name \"$escaped_sound\"" >/dev/null 2>&1; then
+ exit 0
+ fi
else
- osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\"" >/dev/null 2>&1 || true
+ if osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\"" >/dev/null 2>&1; then
+ exit 0
+ fi
fi
-
- exit 0
fi
if command -v notify-send >/dev/null 2>&1; then
- notify-send "$title" "$message" >/dev/null 2>&1 || true
- exit 0
+ if notify-send "$title" "$message" >/dev/null 2>&1; then
+ exit 0
+ fi
fi
if command -v terminal-notifier >/dev/null 2>&1; then
- terminal-notifier -title "$title" -message "$message" >/dev/null 2>&1 || true
- exit 0
+ if terminal-notifier -title "$title" -message "$message" >/dev/null 2>&1; then
+ exit 0
+ fi
fi📝 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.
Suggested change
| if command -v osascript >/dev/null 2>&1; then | |
| escaped_title="$(escape_applescript "$title")" | |
| escaped_message="$(escape_applescript "$message")" | |
| if [[ -n "$sound" ]]; then | |
| escaped_sound="$(escape_applescript "$sound")" | |
| osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\" sound name \"$escaped_sound\"" >/dev/null 2>&1 || true | |
| else | |
| osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\"" >/dev/null 2>&1 || true | |
| fi | |
| exit 0 | |
| fi | |
| if command -v notify-send >/dev/null 2>&1; then | |
| notify-send "$title" "$message" >/dev/null 2>&1 || true | |
| exit 0 | |
| fi | |
| if command -v terminal-notifier >/dev/null 2>&1; then | |
| terminal-notifier -title "$title" -message "$message" >/dev/null 2>&1 || true | |
| exit 0 | |
| fi | |
| if command -v osascript >/dev/null 2>&1; then | |
| escaped_title="$(escape_applescript "$title")" | |
| escaped_message="$(escape_applescript "$message")" | |
| if [[ -n "$sound" ]]; then | |
| escaped_sound="$(escape_applescript "$sound")" | |
| if osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\" sound name \"$escaped_sound\"" >/dev/null 2>&1; then | |
| exit 0 | |
| fi | |
| else | |
| if osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\"" >/dev/null 2>&1; then | |
| exit 0 | |
| fi | |
| fi | |
| fi | |
| if command -v notify-send >/dev/null 2>&1; then | |
| if notify-send "$title" "$message" >/dev/null 2>&1; then | |
| exit 0 | |
| fi | |
| fi | |
| if command -v terminal-notifier >/dev/null 2>&1; then | |
| if terminal-notifier -title "$title" -message "$message" >/dev/null 2>&1; then | |
| exit 0 | |
| fi | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/modules/local-binaries/default.nix` around lines 62 - 84, The
notifier blocks (commands osascript, notify-send, terminal-notifier called in
the script around the escape_applescript usage) currently use "|| true" and then
unconditionally exit, which masks failures and prevents fallbacks; change each
block to run the notifier command without forcing success, capture its exit
status, and only run exit 0 when the command succeeded (e.g., test the command's
return code or use && exit 0 after the notifier invocation) so that if
osascript/notify-send/terminal-notifier exists but fails, the script will
continue to try the next backend instead of silently stopping.
Move clipboard-copy and notify-local bash scripts from inline nix text blocks to standalone .sh files with a dedicated scripts module. Add shellspec tests for both scripts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 0876af77c12f
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 7 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/clipboard_copy_spec.sh">
<violation number="1" location="spec/clipboard_copy_spec.sh:19">
P2: These backend-selection tests only check exit status and never assert which clipboard command was invoked, so they can pass without validating the intended code path.</violation>
<violation number="2" location="spec/clipboard_copy_spec.sh:80">
P2: The no-backend test leaks an entire system bin directory into PATH, so clipboard backends may still be discoverable and make this test flaky/non-deterministic.</violation>
</file>
<file name="spec/notify_local_spec.sh">
<violation number="1" location="spec/notify_local_spec.sh:56">
P2: The terminal-notifier test setup only hides other backends when `osascript` exists, so on Linux it can accidentally use system `notify-send` and not actually test the intended branch.</violation>
</file>
<file name="home-manager/modules/scripts/notify-local.sh">
<violation number="1" location="home-manager/modules/scripts/notify-local.sh:34">
P2: The osascript path always exits even on failure, so fallback notifiers are never attempted when osascript is present but unusable.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -0,0 +1,97 @@ | |||
| #!/usr/bin/env bash | |||
Contributor
There was a problem hiding this comment.
P2: These backend-selection tests only check exit status and never assert which clipboard command was invoked, so they can pass without validating the intended code path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/clipboard_copy_spec.sh, line 19:
<comment>These backend-selection tests only check exit status and never assert which clipboard command was invoked, so they can pass without validating the intended code path.</comment>
<file context>
@@ -0,0 +1,97 @@
+
+It 'uses pbcopy'
+When run bash "$SCRIPT" </dev/null
+The status should be success
+End
+End
</file context>
| # Keep bash on PATH but nothing else | ||
| local bash_dir | ||
| bash_dir="$(dirname "$(command -v bash)")" | ||
| export PATH="$MOCK_BIN:$bash_dir" |
Contributor
There was a problem hiding this comment.
P2: The no-backend test leaks an entire system bin directory into PATH, so clipboard backends may still be discoverable and make this test flaky/non-deterministic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/clipboard_copy_spec.sh, line 80:
<comment>The no-backend test leaks an entire system bin directory into PATH, so clipboard backends may still be discoverable and make this test flaky/non-deterministic.</comment>
<file context>
@@ -0,0 +1,97 @@
+ # Keep bash on PATH but nothing else
+ local bash_dir
+ bash_dir="$(dirname "$(command -v bash)")"
+ export PATH="$MOCK_BIN:$bash_dir"
+ export MOCK_BIN MOCK_ORIGINAL_PATH
+}
</file context>
| setup() { | ||
| mock_bin_setup notify-send | ||
| # Hide osascript so notify-send path is taken | ||
| if command -v osascript >/dev/null 2>&1; then |
Contributor
There was a problem hiding this comment.
P2: The terminal-notifier test setup only hides other backends when osascript exists, so on Linux it can accidentally use system notify-send and not actually test the intended branch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/notify_local_spec.sh, line 56:
<comment>The terminal-notifier test setup only hides other backends when `osascript` exists, so on Linux it can accidentally use system `notify-send` and not actually test the intended branch.</comment>
<file context>
@@ -0,0 +1,128 @@
+setup() {
+ mock_bin_setup notify-send
+ # Hide osascript so notify-send path is taken
+ if command -v osascript >/dev/null 2>&1; then
+ local bash_dir cat_dir printf_dir
+ bash_dir="$(dirname "$(command -v bash)")"
</file context>
| escaped_sound="$(escape_applescript "$sound")" | ||
| osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\" sound name \"$escaped_sound\"" >/dev/null 2>&1 || true | ||
| else | ||
| osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\"" >/dev/null 2>&1 || true |
Contributor
There was a problem hiding this comment.
P2: The osascript path always exits even on failure, so fallback notifiers are never attempted when osascript is present but unusable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/modules/scripts/notify-local.sh, line 34:
<comment>The osascript path always exits even on failure, so fallback notifiers are never attempted when osascript is present but unusable.</comment>
<file context>
@@ -0,0 +1,50 @@
+ escaped_sound="$(escape_applescript "$sound")"
+ osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\" sound name \"$escaped_sound\"" >/dev/null 2>&1 || true
+ else
+ osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\"" >/dev/null 2>&1 || true
+ fi
+
</file context>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@home-manager/modules/scripts/notify-local.sh`:
- Around line 26-48: The fallback logic currently suppresses failures with "||
true" and then unconditionally exits, so if
osascript/notify-send/terminal-notifier exists but fails the script won't try
the next backend; change each backend block (the osascript block that uses
escaped_title/escaped_message/escaped_sound, the notify-send invocation, and the
terminal-notifier invocation) to run the notification command without "|| true"
and only exit 0 when that command returns success (e.g. command && exit 0 or
capture the exit status and conditionally exit), so a runtime failure will fall
through to the next backend.
In `@spec/clipboard_copy_spec.sh`:
- Around line 23-71: The tests for "when wl-copy is available", "when xclip is
available", and "when xsel is available" are non-deterministic because other
higher-priority backends on PATH can satisfy the run; update each setup/cleanup
pair (the setup/cleanup functions used by Describe blocks and the
mock_bin_setup/mock_bin_cleanup helpers) to isolate PATH so only the test's
mocked binary is visible (e.g., create a temp mock bin dir in mock_bin_setup,
export PATH to that dir only during the test, and restore PATH in
mock_bin_cleanup), and apply the same PATH-isolation pattern used for xclip to
the wl-copy and xsel tests and ensure WAYLAND_DISPLAY is set/unset only where
intended.
In `@spec/notify_local_spec.sh`:
- Around line 79-103: The test setup() leaves host notify-send visible so the
script may choose notify-send instead of terminal-notifier; update setup() in
spec/notify_local_spec.sh to ensure notify-send (and osascript) are not on PATH
by either adding mock_bin_setup notify-send (so a dummy is used) or by removing
the directory containing notify-send from PATH (e.g., capture "$(dirname
"$(command -v notify-send)")" and exclude it when exporting PATH), and keep the
existing logic that hides osascript by rebuilding PATH to include only
"$MOCK_BIN" plus required system dirs; also ensure mock_bin_cleanup is still
called in cleanup().
🪄 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: e353040b-04f6-46fd-8e45-a85ec0ddb47d
📒 Files selected for processing (6)
home-manager/modules/default.nixhome-manager/modules/scripts/clipboard-copy.shhome-manager/modules/scripts/default.nixhome-manager/modules/scripts/notify-local.shspec/clipboard_copy_spec.shspec/notify_local_spec.sh
✅ Files skipped from review due to trivial changes (3)
- home-manager/modules/default.nix
- home-manager/modules/scripts/default.nix
- home-manager/modules/scripts/clipboard-copy.sh
Comment on lines
+26
to
+48
| if command -v osascript >/dev/null 2>&1; then | ||
| escaped_title="$(escape_applescript "$title")" | ||
| escaped_message="$(escape_applescript "$message")" | ||
|
|
||
| if [[ -n "$sound" ]]; then | ||
| escaped_sound="$(escape_applescript "$sound")" | ||
| osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\" sound name \"$escaped_sound\"" >/dev/null 2>&1 || true | ||
| else | ||
| osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\"" >/dev/null 2>&1 || true | ||
| fi | ||
|
|
||
| exit 0 | ||
| fi | ||
|
|
||
| if command -v notify-send >/dev/null 2>&1; then | ||
| notify-send "$title" "$message" >/dev/null 2>&1 || true | ||
| exit 0 | ||
| fi | ||
|
|
||
| if command -v terminal-notifier >/dev/null 2>&1; then | ||
| terminal-notifier -title "$title" -message "$message" >/dev/null 2>&1 || true | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
Fallback chain exits even when the chosen backend fails.
Line 32/34/41/46 suppress errors with || true, then Line 37/42/47 unconditionally exits. If osascript or notify-send exists but fails at runtime, the script won’t try the next backend.
Suggested fix
if command -v osascript >/dev/null 2>&1; then
escaped_title="$(escape_applescript "$title")"
escaped_message="$(escape_applescript "$message")"
if [[ -n "$sound" ]]; then
escaped_sound="$(escape_applescript "$sound")"
- osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\" sound name \"$escaped_sound\"" >/dev/null 2>&1 || true
+ if osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\" sound name \"$escaped_sound\"" >/dev/null 2>&1; then
+ exit 0
+ fi
else
- osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\"" >/dev/null 2>&1 || true
+ if osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\"" >/dev/null 2>&1; then
+ exit 0
+ fi
fi
-
- exit 0
fi
if command -v notify-send >/dev/null 2>&1; then
- notify-send "$title" "$message" >/dev/null 2>&1 || true
- exit 0
+ if notify-send "$title" "$message" >/dev/null 2>&1; then
+ exit 0
+ fi
fi
if command -v terminal-notifier >/dev/null 2>&1; then
- terminal-notifier -title "$title" -message "$message" >/dev/null 2>&1 || true
- exit 0
+ terminal-notifier -title "$title" -message "$message" >/dev/null 2>&1 || true
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/modules/scripts/notify-local.sh` around lines 26 - 48, The
fallback logic currently suppresses failures with "|| true" and then
unconditionally exits, so if osascript/notify-send/terminal-notifier exists but
fails the script won't try the next backend; change each backend block (the
osascript block that uses escaped_title/escaped_message/escaped_sound, the
notify-send invocation, and the terminal-notifier invocation) to run the
notification command without "|| true" and only exit 0 when that command returns
success (e.g. command && exit 0 or capture the exit status and conditionally
exit), so a runtime failure will fall through to the next backend.
Comment on lines
+23
to
+71
| Describe 'when wl-copy is available on Wayland' | ||
| setup() { | ||
| mock_bin_setup wl-copy | ||
| export WAYLAND_DISPLAY=wayland-0 | ||
| } | ||
| cleanup() { | ||
| mock_bin_cleanup | ||
| unset WAYLAND_DISPLAY | ||
| } | ||
| Before 'setup' | ||
| After 'cleanup' | ||
|
|
||
| It 'uses wl-copy' | ||
| When run bash "$SCRIPT" </dev/null | ||
| The status should be success | ||
| End | ||
| End | ||
|
|
||
| Describe 'when xclip is available' | ||
| setup() { | ||
| mock_bin_setup xclip | ||
| } | ||
| cleanup() { | ||
| mock_bin_cleanup | ||
| } | ||
| Before 'setup' | ||
| After 'cleanup' | ||
|
|
||
| It 'uses xclip' | ||
| When run bash "$SCRIPT" </dev/null | ||
| The status should be success | ||
| End | ||
| End | ||
|
|
||
| Describe 'when xsel is available' | ||
| setup() { | ||
| mock_bin_setup xsel | ||
| } | ||
| cleanup() { | ||
| mock_bin_cleanup | ||
| } | ||
| Before 'setup' | ||
| After 'cleanup' | ||
|
|
||
| It 'uses xsel' | ||
| When run bash "$SCRIPT" </dev/null | ||
| The status should be success | ||
| End | ||
| End |
There was a problem hiding this comment.
Backend-selection tests are currently non-deterministic.
At Line 23, Line 41, and Line 57 blocks, the tests only assert success, so a higher-priority backend on PATH can satisfy the run and still pass. That means these cases may not actually validate wl-copy/xclip/xsel selection.
Suggested hardening
It 'uses xclip'
-When run bash "$SCRIPT" </dev/null
+When run bash -c 'bash '"$SCRIPT"' </dev/null; cat "$MOCK_LOG"'
The status should be success
+The output should include 'xclip'
EndApply the same pattern to wl-copy and xsel, and isolate PATH similarly where needed so earlier backends cannot be chosen.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/clipboard_copy_spec.sh` around lines 23 - 71, The tests for "when
wl-copy is available", "when xclip is available", and "when xsel is available"
are non-deterministic because other higher-priority backends on PATH can satisfy
the run; update each setup/cleanup pair (the setup/cleanup functions used by
Describe blocks and the mock_bin_setup/mock_bin_cleanup helpers) to isolate PATH
so only the test's mocked binary is visible (e.g., create a temp mock bin dir in
mock_bin_setup, export PATH to that dir only during the test, and restore PATH
in mock_bin_cleanup), and apply the same PATH-isolation pattern used for xclip
to the wl-copy and xsel tests and ensure WAYLAND_DISPLAY is set/unset only where
intended.
Comment on lines
+79
to
+103
| Describe 'with terminal-notifier available (no osascript or notify-send)' | ||
| setup() { | ||
| mock_bin_setup terminal-notifier | ||
| # Hide osascript so terminal-notifier path is taken | ||
| if command -v osascript >/dev/null 2>&1; then | ||
| local bash_dir cat_dir printf_dir | ||
| bash_dir="$(dirname "$(command -v bash)")" | ||
| cat_dir="$(dirname "$(command -v cat)")" | ||
| printf_dir="$(dirname "$(command -v printf)")" | ||
| export PATH="$MOCK_BIN:$bash_dir:$cat_dir:$printf_dir" | ||
| fi | ||
| } | ||
| cleanup() { | ||
| mock_bin_cleanup | ||
| } | ||
| Before 'setup' | ||
| After 'cleanup' | ||
|
|
||
| It 'uses terminal-notifier' | ||
| When run bash -c 'bash '"$SCRIPT"' "Title" "Message"; cat "$MOCK_LOG"' | ||
| The status should be success | ||
| The output should include 'terminal-notifier' | ||
| The output should include 'Title' | ||
| The output should include 'Message' | ||
| End |
There was a problem hiding this comment.
terminal-notifier scenario can be bypassed by host notify-send.
In this setup, notify-send is not isolated out of PATH, so the script may take the notify-send branch instead of terminal-notifier depending on runner environment.
Suggested fix
Describe 'with terminal-notifier available (no osascript or notify-send)'
setup() {
mock_bin_setup terminal-notifier
- # Hide osascript so terminal-notifier path is taken
- if command -v osascript >/dev/null 2>&1; then
- local bash_dir cat_dir printf_dir
- bash_dir="$(dirname "$(command -v bash)")"
- cat_dir="$(dirname "$(command -v cat)")"
- printf_dir="$(dirname "$(command -v printf)")"
- export PATH="$MOCK_BIN:$bash_dir:$cat_dir:$printf_dir"
- fi
+ # Isolate PATH so neither osascript nor notify-send can be discovered
+ local bash_dir cat_dir printf_dir
+ bash_dir="$(dirname "$(command -v bash)")"
+ cat_dir="$(dirname "$(command -v cat)")"
+ printf_dir="$(dirname "$(command -v printf)")"
+ export PATH="$MOCK_BIN:$bash_dir:$cat_dir:$printf_dir"
}📝 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.
Suggested change
| Describe 'with terminal-notifier available (no osascript or notify-send)' | |
| setup() { | |
| mock_bin_setup terminal-notifier | |
| # Hide osascript so terminal-notifier path is taken | |
| if command -v osascript >/dev/null 2>&1; then | |
| local bash_dir cat_dir printf_dir | |
| bash_dir="$(dirname "$(command -v bash)")" | |
| cat_dir="$(dirname "$(command -v cat)")" | |
| printf_dir="$(dirname "$(command -v printf)")" | |
| export PATH="$MOCK_BIN:$bash_dir:$cat_dir:$printf_dir" | |
| fi | |
| } | |
| cleanup() { | |
| mock_bin_cleanup | |
| } | |
| Before 'setup' | |
| After 'cleanup' | |
| It 'uses terminal-notifier' | |
| When run bash -c 'bash '"$SCRIPT"' "Title" "Message"; cat "$MOCK_LOG"' | |
| The status should be success | |
| The output should include 'terminal-notifier' | |
| The output should include 'Title' | |
| The output should include 'Message' | |
| End | |
| Describe 'with terminal-notifier available (no osascript or notify-send)' | |
| setup() { | |
| mock_bin_setup terminal-notifier | |
| # Isolate PATH so neither osascript nor notify-send can be discovered | |
| local bash_dir cat_dir printf_dir | |
| bash_dir="$(dirname "$(command -v bash)")" | |
| cat_dir="$(dirname "$(command -v cat)")" | |
| printf_dir="$(dirname "$(command -v printf)")" | |
| export PATH="$MOCK_BIN:$bash_dir:$cat_dir:$printf_dir" | |
| } | |
| cleanup() { | |
| mock_bin_cleanup | |
| } | |
| Before 'setup' | |
| After 'cleanup' | |
| It 'uses terminal-notifier' | |
| When run bash -c 'bash '"$SCRIPT"' "Title" "Message"; cat "$MOCK_LOG"' | |
| The status should be success | |
| The output should include 'terminal-notifier' | |
| The output should include 'Title' | |
| The output should include 'Message' | |
| End |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/notify_local_spec.sh` around lines 79 - 103, The test setup() leaves
host notify-send visible so the script may choose notify-send instead of
terminal-notifier; update setup() in spec/notify_local_spec.sh to ensure
notify-send (and osascript) are not on PATH by either adding mock_bin_setup
notify-send (so a dummy is used) or by removing the directory containing
notify-send from PATH (e.g., capture "$(dirname "$(command -v notify-send)")"
and exclude it when exporting PATH), and keep the existing logic that hides
osascript by rebuilding PATH to include only "$MOCK_BIN" plus required system
dirs; also ensure mock_bin_cleanup is still called in cleanup().
…rectory Entire-Checkpoint: da954c1272e9
Update all references to clipboard-copy and notify-local to use ~/.local/scripts/ instead of ~/.local/bin/. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 8075bc53fe6a
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 45f07761fccf
Entire-Checkpoint: b4199942d951
…otification scripts Entire-Checkpoint: f65f116ce65c
Entire-Checkpoint: c7216e83c8b1
Contributor
There was a problem hiding this comment.
1 issue found across 5 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:232">
P2: The new local-scripts entries were added to `covered_scripts` without adding matching `has spec file for ...` assertions, weakening this test’s spec-file coverage guard.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
GTK_THEME=Adwaita:dark in hyprland.conf was overriding the portal color-scheme preference, keeping Electron apps (Slack, Chrome) stuck in dark mode. Extracted darkman scripts to external files and added spec coverage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
2 issues found across 6 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/darkman_spec.sh">
<violation number="1" location="spec/darkman_spec.sh:55">
P2: The light-theme assertion is too broad and can pass for `Adwaita-dark`; assert the exact light value instead.</violation>
</file>
<file name="home-manager/services/darkman/default.nix">
<violation number="1" location="home-manager/services/darkman/default.nix:12">
P2: The extraction to `builtins.readFile` dropped explicit `${pkgs.dconf}/bin/dconf` resolution, so these hooks now depend on `dconf` being present on PATH and may fail in sessions where it is not.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| It 'sets gtk-theme to Adwaita' | ||
| When run bash -c "cat '$LIGHT_SCRIPT'" | ||
| The output should include "Adwaita" |
Contributor
There was a problem hiding this comment.
P2: The light-theme assertion is too broad and can pass for Adwaita-dark; assert the exact light value instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/darkman_spec.sh, line 55:
<comment>The light-theme assertion is too broad and can pass for `Adwaita-dark`; assert the exact light value instead.</comment>
<file context>
@@ -0,0 +1,64 @@
+
+It 'sets gtk-theme to Adwaita'
+When run bash -c "cat '$LIGHT_SCRIPT'"
+The output should include "Adwaita"
+End
+
</file context>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
home-manager/modules/local-scripts/notify-local.sh (1)
26-48:⚠️ Potential issue | 🟠 MajorFallback chain still exits even when a backend fails.
On Line 37 / Line 42 / Line 47, the script exits after backend detection, not backend success. Runtime failures are swallowed and no fallback is attempted.
Proposed fix
if command -v osascript >/dev/null 2>&1; then escaped_title="$(escape_applescript "$title")" escaped_message="$(escape_applescript "$message")" if [[ -n $sound ]]; then escaped_sound="$(escape_applescript "$sound")" - osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\" sound name \"$escaped_sound\"" >/dev/null 2>&1 || true + if osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\" sound name \"$escaped_sound\"" >/dev/null 2>&1; then + exit 0 + fi else - osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\"" >/dev/null 2>&1 || true + if osascript -e "display notification \"$escaped_message\" with title \"$escaped_title\"" >/dev/null 2>&1; then + exit 0 + fi fi - - exit 0 fi if command -v notify-send >/dev/null 2>&1; then - notify-send "$title" "$message" >/dev/null 2>&1 || true - exit 0 + if notify-send "$title" "$message" >/dev/null 2>&1; then + exit 0 + fi fi if command -v terminal-notifier >/dev/null 2>&1; then - terminal-notifier -title "$title" -message "$message" >/dev/null 2>&1 || true - exit 0 + if terminal-notifier -title "$title" -message "$message" >/dev/null 2>&1; then + exit 0 + fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/modules/local-scripts/notify-local.sh` around lines 26 - 48, The script currently exits after selecting a backend even if the notification command fails, because each backend block ends with an unconditional exit and uses "|| true" to swallow runtime failures; update each backend block (the osascript display notification branch, the notify-send branch, and the terminal-notifier branch) to attempt the notification, check its exit status, and only exit 0 when the command actually succeeds—otherwise continue to the next backend. Concretely: remove the "|| true" and the unconditional exit inside each detected-backend block, run the notification (redirecting output as needed), test the command's return code (e.g. if command succeeds then exit 0), and only fall through to subsequent backends when the notification failed.
🧹 Nitpick comments (1)
spec/darkman_spec.sh (1)
53-56: Light-mode GTK theme assertion is too broad.Current check can pass on non-exact values containing
Adwaita. Prefer matching the quoted exact theme token.Proposed test tightening
It 'sets gtk-theme to Adwaita' When run bash -c "cat '$LIGHT_SCRIPT'" -The output should include "Adwaita" +The output should include "'Adwaita'" End🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/darkman_spec.sh` around lines 53 - 56, The test "It 'sets gtk-theme to Adwaita'" in spec/darkman_spec.sh currently checks for a bare Adwaita token which is too broad; update the assertion that runs bash -c "cat '$LIGHT_SCRIPT'" to look for the exact quoted theme token (e.g. match '"Adwaita"' or use a regex with word boundaries like \bAdwaita\b) so only an exact theme value passes; modify the expectation line that currently uses The output should include "Adwaita" to assert the exact quoted token instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@home-manager/modules/local-scripts/notify-local.sh`:
- Around line 26-48: The script currently exits after selecting a backend even
if the notification command fails, because each backend block ends with an
unconditional exit and uses "|| true" to swallow runtime failures; update each
backend block (the osascript display notification branch, the notify-send
branch, and the terminal-notifier branch) to attempt the notification, check its
exit status, and only exit 0 when the command actually succeeds—otherwise
continue to the next backend. Concretely: remove the "|| true" and the
unconditional exit inside each detected-backend block, run the notification
(redirecting output as needed), test the command's return code (e.g. if command
succeeds then exit 0), and only fall through to subsequent backends when the
notification failed.
---
Nitpick comments:
In `@spec/darkman_spec.sh`:
- Around line 53-56: The test "It 'sets gtk-theme to Adwaita'" in
spec/darkman_spec.sh currently checks for a bare Adwaita token which is too
broad; update the assertion that runs bash -c "cat '$LIGHT_SCRIPT'" to look for
the exact quoted theme token (e.g. match '"Adwaita"' or use a regex with word
boundaries like \bAdwaita\b) so only an exact theme value passes; modify the
expectation line that currently uses The output should include "Adwaita" to
assert the exact quoted token instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a35bfc8c-2364-446a-8fb2-9ca51bf1e684
📒 Files selected for processing (20)
config/claude/notify.shconfig/codex/config.tomlconfig/codex/config.tpl.tomlconfig/hyprland/hyprland.confhome-manager/modules/default.nixhome-manager/modules/local-scripts/clipboard-copy.shhome-manager/modules/local-scripts/default.nixhome-manager/modules/local-scripts/notify-local.shhome-manager/programs/bash/default.nixhome-manager/programs/fish/default.nixhome-manager/programs/tmux/tmux.confhome-manager/programs/zsh/default.nixhome-manager/services/darkman/dark-mode.shhome-manager/services/darkman/default.nixhome-manager/services/darkman/light-mode.shspec/clipboard_copy_spec.shspec/coverage_spec.shspec/darkman_spec.shspec/notify_local_spec.shspec/notify_spec.sh
💤 Files with no reviewable changes (1)
- config/hyprland/hyprland.conf
✅ Files skipped from review due to trivial changes (7)
- home-manager/programs/zsh/default.nix
- config/codex/config.tpl.toml
- home-manager/programs/bash/default.nix
- home-manager/services/darkman/light-mode.sh
- home-manager/services/darkman/dark-mode.sh
- home-manager/modules/local-scripts/clipboard-copy.sh
- home-manager/modules/local-scripts/default.nix
🚧 Files skipped from review as they are similar to previous changes (6)
- config/codex/config.toml
- home-manager/modules/default.nix
- spec/clipboard_copy_spec.sh
- spec/notify_local_spec.sh
- home-manager/programs/tmux/tmux.conf
- home-manager/services/darkman/default.nix
The darkman mode scripts were failing with "dconf: command not found" because builtins.readFile doesn't interpolate Nix store paths. Use builtins.replaceStrings with @dconf@ placeholders matching the existing pattern in config/ccs/default.nix. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Adds cross-platform notification and clipboard helpers, wires them into
claude,codex, andtmux, and enables automatic light/dark switching on Linux withdarkmanand customghosttythemes. Scripts now live in~/.local/scripts, are on PATH forbash,fish, andzsh, and include tests across macOS and Linux.New Features
~/.local/scripts/notify-local: unified notifier for macOS (osascript), Linux (notify-send), andterminal-notifier; used byclaudeandcodexwith PATH/~/.local/scriptsfallback.~/.local/scripts/clipboard-copy: pipe topbcopy,wl-copy,xclip, orxsel; used bytmux.darkmanservice with GTK/dconf scripts and XDG portal integration;ghosttynow usesdark:Dracula Custom,light:Catppuccin Latte Custom.Bug Fixes
darkmantheming: removedGTK_THEME=Adwaita:darkand GTK “prefer-dark” overrides, and ensured scripts finddconfvia@dconf@so portal-aware apps follow system light/dark mode.Written for commit 576d953. Summary will update on new commits.