fix: resolve Lua migration regressions - #1879
Conversation
Restore the documented HyDE keymap in the Lua configuration, fix the all-monitors screenshot action, and isolate app2unit from unrelated DEBUG values. Add regression coverage for the public shortcuts and app wrapper environment.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe app wrapper now isolates debug variables. Hyprland Lua shortcuts, battery notification defaults, Satty rendering, Grimblast sourcing, documentation, changelog entries, and regression tests are updated. ChangesRuntime and keybinding corrections
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![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)
tests/test_app_wrapper.sh (1)
8-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the non-systemd branch instead of skipping it.
When systemd is unavailable, this test exits successfully without running any assertions. That leaves the direct
exec "$@"path—the path where the wrapper currently fails to export the scoped debug defaults—untested. Add a fallback-path case, including thexdg-terminal-execconsumer documented intests/README.mdLine 22, or guarantee a complementary non-systemd CI job.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_app_wrapper.sh` around lines 8 - 11, Update the systemd availability guard in the test setup so unavailable systemd does not simply skip coverage; add a non-systemd fallback case that exercises the wrapper’s direct exec "$@" path, including the xdg-terminal-exec consumer documented in tests/README.md, or provide an equivalent dedicated non-systemd CI job.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Configs/.local/lib/hyde/app.sh`:
- Around line 9-15: Move the APP2UNIT_DEBUG and XTE_DEBUG default assignments
and export in app.sh above the sys availability check, so both the systemd path
and the direct exec "$@" path initialize scoped values while preserving
user-provided settings.
In `@tests/lua/bind_harness.lua`:
- Around line 242-280: Add the omitted restored shortcuts to the
required_defaults table: SUPER + SHIFT + G, SUPER + SHIFT + P, SUPER + CTRL + S,
SUPER + SHIFT + W, SUPER + SHIFT + R, and SUPER + SHIFT + T. Extend the
screenshot_commands coverage so the relevant OCR shortcut dispatches hyde-shell
screenshot sc, preserving the existing canonical lookup and assertion patterns.
---
Nitpick comments:
In `@tests/test_app_wrapper.sh`:
- Around line 8-11: Update the systemd availability guard in the test setup so
unavailable systemd does not simply skip coverage; add a non-systemd fallback
case that exercises the wrapper’s direct exec "$@" path, including the
xdg-terminal-exec consumer documented in tests/README.md, or provide an
equivalent dedicated non-systemd CI job.
🪄 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 Plus
Run ID: ff830570-af8e-4851-8362-6699135510e5
📒 Files selected for processing (7)
CHANGELOG.mdConfigs/.local/lib/hyde/app.shConfigs/.local/share/hypr/lua/key_binds.luaKEYBINDINGS.mdtests/README.mdtests/lua/bind_harness.luatests/test_app_wrapper.sh
Apply scoped debug defaults before selecting the systemd or direct execution path. Exercise xdg-terminal-exec without systemd and extend the restored keybinding assertions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_app_wrapper.sh (1)
46-58: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftMake systemd-path coverage independent of the host.
On non-systemd CI, the only runtime assertions for
app2unitare skipped. The source-order check does not prove that the systemd branch exports the correct values; add a test seam or otherwise exercise both execution paths explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_app_wrapper.sh` around lines 46 - 58, The test currently skips all runtime assertions when systemd is unavailable, so it does not verify both app2unit execution paths. Update the test around the systemd detection and wrapper invocation to add a seam or controlled override that exercises the systemd and non-systemd branches independently, then assert APP2UNIT_DEBUG and XTE_DEBUG default to 0 for each path while retaining relevant ordering validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_app_wrapper.sh`:
- Around line 30-44: Add a test case in the app wrapper test around the existing
DEBUG environment setup that exports explicit nonzero APP2UNIT_DEBUG and
XTE_DEBUG values before invoking the wrapper. Assert the corresponding
APP2UNIT_CONSUMER_DEBUG and XTE_CONSUMER_DEBUG outputs preserve those
caller-provided values, while retaining the existing unset-variable coverage.
---
Nitpick comments:
In `@tests/test_app_wrapper.sh`:
- Around line 46-58: The test currently skips all runtime assertions when
systemd is unavailable, so it does not verify both app2unit execution paths.
Update the test around the systemd detection and wrapper invocation to add a
seam or controlled override that exercises the systemd and non-systemd branches
independently, then assert APP2UNIT_DEBUG and XTE_DEBUG default to 0 for each
path while retaining relevant ordering validation.
🪄 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 Plus
Run ID: 2ec93be4-b5b2-467b-b092-cbcc9c875a38
📒 Files selected for processing (4)
Configs/.local/lib/hyde/app.shtests/README.mdtests/lua/bind_harness.luatests/test_app_wrapper.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/README.md
- Configs/.local/lib/hyde/app.sh
- tests/lua/bind_harness.lua
Cover caller-provided APP2UNIT_DEBUG and XTE_DEBUG values in addition to the unrelated DEBUG fallback regression.
Keep generated desktop startup defaults aligned with the Lua implementation installed by the migration. Add regression coverage so removed shell commands cannot return through generated schema artifacts.
Replace the stale experimental toplevel branch with upstream main, which performs a single slurp selection and avoids combining monitor geometry with window capture. Guard the installer source with the dotfile regression test.
Default Satty to the compatible GSK GL renderer when launched from Hyprland while preserving explicit user overrides. Add wrapper regression coverage.
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Description
This fixes several regressions that remained after the Hyprland Lua configuration migration:
key_binds.lua, including screenshots, scratchpads, workspace scrolling, media keys, selectors, grouping, and resize actions;Printkey call the full-screen screenshot action instead of the monitor action;slurpprompt and invalid mixed window/monitor geometry;GSK_RENDERERoverride is configured, preventing a black annotation preview when launched from Hyprland;hyde-shell appfrom an unrelated exportedDEBUGvalue by providing explicit defaults forAPP2UNIT_DEBUGandXTE_DEBUG;batterynotify.lua, replacing the removedbatterynotify.shcommand;The app-wrapper failure can be reproduced by launching an application through
hyde-shell appfrom an environment that exportsDEBUG=release.app2unitpreviously interpreted the generic value as a boolean and emitted:The screenshot regression can be reproduced by pressing
Print: the Lua binding previously dispatchedhyde-shell screenshot m(monitor) rather thanhyde-shell screenshot p(full screen).The area-capture regression can be reproduced with
Super + P: the pinned experimental Grimblast branch invokesslurponce for the output and then again for the actual region/window. For window choices it also combines the first output geometry with a toplevel handle. The corrected official upstream performs one selection and passes either region geometry or the selected window handle.After that selection completes, Satty can open as a fully black window when launched through Hyprland's Lua dispatcher with the default GTK renderer. Running the same command with
GSK_RENDERER=glrenders both the captured image and Satty's toolbars correctly. The wrapper now supplies that compatible default only for Satty and preserves any explicit user override.After the migration moves the superseded
batterynotify.shaside, a config generated from the committed schema still tries to start it. This produces anExecutable not found: 'batterynotify.sh'desktop notification even though the replacementbatterynotify.luais installed and the Lua defaults already reference it.The existing
v26.7.4migration ondevalready handles obsolete selector scripts; this PR addresses the remaining runtime and keybinding regressions.Related: #1863
No new dependencies are required.
Type of change
Checklist
Validation
sh tests/run.sh: 8 cases, 0 failuresHyprland --verify-config:config okslurpinvocation and a valid non-black 1920x1080 PNGorigin/devbaseline and pass with this patchScreenshots
Not applicable; these are configuration, command-dispatch, and environment-handling fixes.
Additional context
The restored combinations follow the repository's published
KEYBINDINGS.mdbehavior. The regression tests assert both key/modifier presence and the exact screenshot command dispatched by each shortcut.Summary by CodeRabbit
Bug Fixes
DEBUGcontains non-boolean values.Documentation
Tests