fix: use ~/.config/vader5/config.toml as default config path - #27
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughUpdated docs and install script; added filesystem-aware config path resolution; refactored gamepad suppression state into a single SuppressState struct with a new apply(...) method and consolidated suppression logic across poll/emission paths. Changes
Sequence Diagram(s)(omitted — changes are internal refactors and docs/install updates; no multi-component sequential flow requiring a diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
BANANASJIM
marked this pull request as ready for review
March 13, 2026 10:25
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install/install.sh`:
- Line 74: The uninstall script currently runs the destructive command "sudo rm
-rf /etc/vader5"; replace that with a safe cleanup that does not recursively
delete user files: maintain a list/variable of the specific installed files and
config names the installer created (referenced by the uninstall/remove block in
install/install.sh), remove only those entries, or move the whole directory to a
timestamped backup and prompt for explicit user confirmation (or require a
--force flag) before deleting; also check ownership and only remove files owned
by the package/install user to avoid deleting third‑party/user data.
In `@src/config.cpp`:
- Around line 186-190: The current else-if using std::getenv("XDG_CONFIG_HOME")
and else if std::getenv("HOME") causes HOME/.config to be skipped when
XDG_CONFIG_HOME exists but doesn't contain the file; change the logic so you
first set base to XDG_CONFIG_HOME if present (using
std::getenv("XDG_CONFIG_HOME") and the variable base) but still attempt
HOME/.config (std::getenv("HOME")) as a fallback if the file is not found under
XDG_CONFIG_HOME — i.e., don't use else if, instead build a search order (try
base = XDG_CONFIG_HOME, then base = HOME + "/.config") or check file existence
under XDG_CONFIG_HOME and only then fall back to HOME/.config; apply the same
change to the similar block around the code referenced at 192-197.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb9a23ed-03c3-4c02-a77d-b2caacfc3adb
📒 Files selected for processing (4)
README.mddocs/configuration.mdinstall/install.shsrc/config.cpp
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 `@install/install.sh`:
- Around line 54-58: The installer currently writes /etc/vader5/config.toml when
missing but the uninstall path always deletes it; change install/install.sh to
create a marker file (e.g. /etc/vader5/.vader5_installed) whenever the script
copies config (the block that runs the sudo cp "$PROJECT_DIR/config/config.toml"
... and calls success "System config created at /etc/vader5/config.toml") and
update the uninstall logic (the block that removes /etc/vader5/config.toml) to
only remove /etc/vader5/config.toml if that marker exists, then remove the
marker when deleting; this ensures pre-existing configs are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e1272fd-d938-4758-9419-0ee23003de5e
📒 Files selected for processing (2)
install/install.shsrc/config.cpp
Comment on lines
+54
to
+58
| if [[ ! -f /etc/vader5/config.toml ]]; then | ||
| sudo mkdir -p /etc/vader5 | ||
| sudo cp "$PROJECT_DIR/config/config.toml" /etc/vader5/config.toml | ||
| success "System config created at /etc/vader5/config.toml" | ||
| fi |
There was a problem hiding this comment.
Do not delete pre-existing system config on uninstall.
Line 54 only creates /etc/vader5/config.toml when absent, but Line 74 always deletes it. This can remove a config that existed before install.
Suggested fix (track installer-owned config with a marker)
install_systemd() {
info "Installing systemd service (requires sudo)..."
if [[ ! -f /etc/vader5/config.toml ]]; then
sudo mkdir -p /etc/vader5
sudo cp "$PROJECT_DIR/config/config.toml" /etc/vader5/config.toml
+ sudo touch /etc/vader5/.managed-by-vader5-installer
success "System config created at /etc/vader5/config.toml"
fi
@@
uninstall() {
@@
- sudo rm -f /etc/vader5/config.toml
- sudo rmdir /etc/vader5 2>/dev/null || true
+ if [[ -f /etc/vader5/.managed-by-vader5-installer ]]; then
+ sudo rm -f /etc/vader5/config.toml /etc/vader5/.managed-by-vader5-installer
+ sudo rmdir /etc/vader5 2>/dev/null || true
+ fiAlso applies to: 74-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install/install.sh` around lines 54 - 58, The installer currently writes
/etc/vader5/config.toml when missing but the uninstall path always deletes it;
change install/install.sh to create a marker file (e.g.
/etc/vader5/.vader5_installed) whenever the script copies config (the block that
runs the sudo cp "$PROJECT_DIR/config/config.toml" ... and calls success "System
config created at /etc/vader5/config.toml") and update the uninstall logic (the
block that removes /etc/vader5/config.toml) to only remove
/etc/vader5/config.toml if that marker exists, then remove the marker when
deleting; this ensures pre-existing configs are preserved.
default_path() now checks ~/.config/vader5/config.toml first before falling back to the relative config/config.toml. This fixes the systemd service not finding the config since its working directory is not the project root. Also fix README: remove incorrect systemctl enable instruction since the service is auto-started by udev rules. Closes #24
Search order: XDG_CONFIG_HOME → ~/.config → /etc/vader5 → relative. Install systemd service config to /etc/vader5/config.toml so the daemon finds it when HOME is unset. Update docs to document the search order.
- Check XDG_CONFIG_HOME and ~/.config as separate fallbacks so both paths are tried even when XDG_CONFIG_HOME is set - Handle XDG_CONFIG_HOME="" per XDG spec (treat as unset) - Use rm -f + rmdir instead of rm -rf to preserve user files
BANANASJIM
force-pushed
the
fix/config-default-path
branch
from
March 13, 2026 21:00
12795dd to
ad18293
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
78-78:⚠️ Potential issue | 🟠 MajorDocument the new default config path and search order.
The PR title and description highlight that the default config path now checks
~/.config/vader5/config.tomlfirst before falling back toconfig/config.toml, but this README still only documents the old relative path. Users following the Quick Start won't discover the per-user config location.Consider adding a brief note here about the config search order (e.g.,
~/.config/vader5/config.toml→/etc/vader5/config.toml→config/config.toml) or at least mention that multiple locations are checked. This would improve discoverability of the new user-friendly default path that is central to this PR.📝 Suggested documentation improvement
## Configuration -Config: `config/config.toml` +The daemon searches for config files in this order: +1. `~/.config/vader5/config.toml` (per-user) +2. `/etc/vader5/config.toml` (system-wide) +3. `config/config.toml` (repository fallback) + +Or specify a custom path with `-c`:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 78, Update the README entry that currently shows only "Config: `config/config.toml`" to document the new default per-user location and full search order: mention `~/.config/vader5/config.toml` as the primary default, then `/etc/vader5/config.toml`, and finally the repository-local `config/config.toml`, or at minimum add a short sentence stating that multiple locations are searched in that order; edit the line containing "Config: `config/config.toml`" (or the surrounding paragraph) to include this note so users discover the new per-user config path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@README.md`:
- Line 78: Update the README entry that currently shows only "Config:
`config/config.toml`" to document the new default per-user location and full
search order: mention `~/.config/vader5/config.toml` as the primary default,
then `/etc/vader5/config.toml`, and finally the repository-local
`config/config.toml`, or at minimum add a short sentence stating that multiple
locations are searched in that order; edit the line containing "Config:
`config/config.toml`" (or the surrounding paragraph) to include this note so
users discover the new per-user config path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09525e00-3788-47e5-8843-0cc9632023fe
📒 Files selected for processing (4)
README.mddocs/configuration.mdinstall/install.shsrc/config.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- install/install.sh
- src/config.cpp
- docs/configuration.md
Extract duplicate left/right stick processing into lambdas.
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
1 participant
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
default_path()now checks~/.config/vader5/config.tomlfirst before falling back to relativeconfig/config.tomlsudo systemctl enable --now vader5dfrom README — service is auto-started by udev rulesSuppressStatestruct, extract duplicate stick processing into lambdasCloses #24
Summary by CodeRabbit
Release Notes
Documentation
Features