feat: add pixi start wrapper script that forwards app args#25
Conversation
📝 WalkthroughWalkthroughThe changes refactor the pixi start task from an inline bash command to a delegated script ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12–15 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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: 0
🧹 Nitpick comments (1)
scripts/start.sh (1)
10-34: Preset parsing is robust with appropriate fallback.The Python-based JSON parsing correctly filters hidden and i686 presets. The fallback list provides resilience when Python or the presets file is unavailable.
Consider adding a comment explaining that the fallback list (lines 32-34) should be kept in sync with
CMakePresets.jsonwhen new presets are added, to help maintainers remember to update both locations.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.mdpixi.tomlscripts/help.shscripts/start.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-29T00:55:08.857Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2025-12-29T00:55:08.857Z
Learning: Applies to docs/**/CMakePresets.json : Use CMake presets for all builds and tests; never run `cmake -B build` or build directly without a preset
Applied to files:
scripts/start.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Static Analysis (clang-tidy)
- GitHub Check: Build and Test (test preset)
🔇 Additional comments (16)
README.md (2)
100-102: LGTM: Documentation accurately reflects the new CLI syntax.The command syntax and default preset behavior are clearly documented and align with the implementation in
scripts/start.sh.
105-106: LGTM: Clear and practical examples.The examples effectively demonstrate both default and explicit preset usage patterns.
scripts/help.sh (2)
14-14: LGTM: Clear command syntax documentation.The help text accurately reflects the new CLI interface with proper notation conventions.
25-28: LGTM: Complete argument documentation.All arguments are clearly documented with appropriate defaults and descriptions.
pixi.toml (2)
62-64: LGTM: Clean refactor to script delegation.Delegating to
scripts/start.shsimplifies the task definition. The script properly handles preset validation and building viapixi run dev.
66-68: LGTM: Explicit script delegation.Making the shader-fetch task explicitly delegate to a script improves consistency with other tasks.
scripts/start.sh (10)
1-9: LGTM: Robust initialization with good bash practices.The script uses strict error handling (
set -euo pipefail) and robust path resolution. Variable initialization is clean and follows best practices.
36-47: LGTM: Comprehensive usage documentation.The usage function provides clear examples covering common cases and edge cases (using
--separator). This aligns well with the help text and README.
49-71: LGTM: Well-structured helper functions.The helper functions follow bash best practices with proper error handling, stderr output, and informative error messages.
73-103: LGTM: Robust argument parsing with edge case handling.The argument parsing correctly handles multiple flag formats (
--preset VALUE,--preset=VALUE), help flag, and the--separator for disambiguation. State tracking withpreset_explicitandpositional_preset_allowedis well-designed.
105-109: LGTM: Correct positional preset detection logic.The conditional logic properly handles the optional positional preset with appropriate guards and validation. The state management correctly updates
preset_explicit.
111-120: LGTM: Proper validation and argument extraction.The validation logic correctly ensures required arguments are present and valid before proceeding. Error messages are helpful by showing usage.
122-128: LGTM: Proper build orchestration and validation.The script correctly uses
pixi run dev(which honors CMake presets per project policies) and validates the resulting binary before use.
130-137: LGTM: Robust cleanup handler.The cleanup function defensively checks process existence before termination and properly reaps the child process. The trap covers all relevant exit scenarios (EXIT, INT, TERM).
139-144: Verify viewer startup before launching app.The script launches the viewer and waits 1 second but doesn't verify it started successfully. If the viewer crashes during initialization, the app will run uncaptured without warning.
Consider adding a check after the sleep to verify the viewer process is still running:
"$VIEWER_BIN" & VIEWER_PID=$! # Give the viewer a moment to initialize before attaching. sleep 1 + +if ! kill -0 "$VIEWER_PID" 2>/dev/null; then + die "viewer process failed to start" +fiAlternatively, if there's a way to detect when the viewer is ready (e.g., checking for a socket file or listening port), that would be more robust than a fixed sleep duration.
145-150: LGTM: Correct exit status propagation.The script properly disables exit-on-error to capture the app's exit status and propagates it correctly. Array expansion is safe in bash even when empty.
1 participant
Summary by CodeRabbit
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.