Update Launch UX With Default + Detach Modes#36
Conversation
📝 WalkthroughWalkthroughThe pull request adds Default and Detach launch modes for the Goggles viewer: Default spawns and manages a target app with environment wiring (GOGGLES_CAPTURE, GOGGLES_WSI_PROXY, DISPLAY, WAYLAND_DISPLAY, optional GOGGLES_WIDTH/HEIGHT) and monitors its lifecycle; Detach runs the viewer standalone. CLI and start script parsing were refactored; Application gains display accessors. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI Parser
participant App as Application
participant Viewer as Viewer Loop
participant Target as Target App Process
participant Monitor as Process Monitor
User->>CLI: goggles -- ./target arg1 arg2
CLI->>CLI: Parse args (default mode), validate separator & command
CLI->>App: Start with detach=false, app dimensions?
App->>App: Enable input forwarding
App->>Viewer: Start viewer event loop
Viewer->>Target: spawn_target_app() with env (DISPLAY, WAYLAND_DISPLAY, GOGGLES_*)
Target->>Target: Execute target app
Viewer->>Monitor: Poll/monitor child process
Target->>Monitor: Exit with status
Monitor->>Viewer: Notify child exit
Viewer->>Viewer: Shutdown viewer loop
Viewer->>Target: Ensure termination if still running
Viewer->>User: Return target app exit code
sequenceDiagram
actor User
participant CLI as CLI Parser
participant App as Application
participant Viewer as Viewer Loop
User->>CLI: goggles --detach --config myconfig.toml
CLI->>CLI: Parse args (detach mode), validate no app command/widths
CLI->>App: Start with detach=true
App->>App: Disable input forwarding
App->>Viewer: Start viewer event loop (no target)
User->>Viewer: Interact and close viewer
Viewer->>User: Exit gracefully
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Streamline the user workflow by making Goggles launch the target app directly with capture. - Add `--detach` flag for viewer-only mode (manual target launch) - Default mode spawns target app with proper capture and compositor environment - Add `--app-width` and `--app-height` CLI options for WSI proxy control - Implement process lifecycle management (exit when app exits or terminate app on close) - Require `--` separator to avoid parsing app args as viewer options - Add CLI parsing tests and update all documentation
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||||
![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
🤖 Fix all issues with AI agents
In @openspec/changes/update-launch-modes-default-detach/proposal.md:
- Around line 9-21: This proposal introduces breaking CLI behavior (new default
vs. detach modes, enforced input forwarding, rejected options like
--app-width/--app-height and implicit GOGGLES_WSI_PROXY), so update the "What
Changes" section to explicitly mark these items with **BREAKING**; specifically
annotate that default mode changes CLI semantics (requirement of `--` separator,
implicit `GOGGLES_WSI_PROXY=1`, setting of `DISPLAY`/`WAYLAND_DISPLAY`, and
support for `--app-width`/`--app-height`) and that detach mode rejects those
options (e.g., `--app-width`, `--app-height`, input forwarding disabled), and
confirm config interaction remains via `config/goggles.toml` with `--config`
overrides.
🧹 Nitpick comments (2)
src/app/main.cpp (2)
60-67: Consider adding a timeout before escalating to SIGKILL.If the child process ignores
SIGTERM,waitpidwill block indefinitely. A robust pattern would wait with a timeout and escalate toSIGKILLif needed.♻️ Optional: Add timeout-based escalation
static auto terminate_child(pid_t pid) -> void { if (pid <= 0) { return; } kill(pid, SIGTERM); - int status = 0; - waitpid(pid, &status, 0); + int status = 0; + for (int i = 0; i < 50; ++i) { // 5 second timeout + if (waitpid(pid, &status, WNOHANG) == pid) { + return; + } + usleep(100000); // 100ms + } + kill(pid, SIGKILL); + waitpid(pid, &status, 0); }
156-172: Redundant condition check.The condition
if (enable_input_forwarding)on line 157 is always true in default mode sinceenable_input_forwarding = !cli_opts.detach(line 134), and we're already inside theif (!cli_opts.detach)block. This nested check can be removed to simplify the logic.♻️ Simplify by removing redundant condition
if (!cli_opts.detach) { - if (enable_input_forwarding) { - const auto x11_display = app->x11_display(); - const auto wayland_display = app->wayland_display(); - - auto spawn_result = - spawn_target_app(cli_opts.app_command, x11_display, wayland_display, - cli_opts.app_width, cli_opts.app_height); - if (!spawn_result) { - GOGGLES_LOG_CRITICAL("Failed to launch target app: {} ({})", - spawn_result.error().message, - goggles::error_code_name(spawn_result.error().code)); - return EXIT_FAILURE; - } - child_pid = spawn_result.value(); - GOGGLES_LOG_INFO("Launched target app (pid={})", child_pid); + const auto x11_display = app->x11_display(); + const auto wayland_display = app->wayland_display(); + + auto spawn_result = + spawn_target_app(cli_opts.app_command, x11_display, wayland_display, + cli_opts.app_width, cli_opts.app_height); + if (!spawn_result) { + GOGGLES_LOG_CRITICAL("Failed to launch target app: {} ({})", + spawn_result.error().message, + goggles::error_code_name(spawn_result.error().code)); + return EXIT_FAILURE; } + child_pid = spawn_result.value(); + GOGGLES_LOG_INFO("Launched target app (pid={})", child_pid);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
README.mdconfig/goggles.tomldocs/input_forwarding.mdopenspec/changes/update-launch-modes-default-detach/design.mdopenspec/changes/update-launch-modes-default-detach/proposal.mdopenspec/changes/update-launch-modes-default-detach/specs/app-window/spec.mdopenspec/changes/update-launch-modes-default-detach/tasks.mdscripts/task/start.shsrc/app/application.cppsrc/app/application.hppsrc/app/cli.hppsrc/app/main.cpptests/CMakeLists.txttests/app/test_cli.cpp
🧰 Additional context used
📓 Path-based instructions (3)
openspec/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/**/*.md: Use#### Scenario:format (4 hashtags) for scenario headers in requirements, not bullets or bold text, with at least one scenario per requirement
Use format- **WHEN** [condition] - **THEN** [expected result]for scenario steps in requirements
Files:
openspec/changes/update-launch-modes-default-detach/proposal.mdopenspec/changes/update-launch-modes-default-detach/specs/app-window/spec.mdopenspec/changes/update-launch-modes-default-detach/tasks.mdopenspec/changes/update-launch-modes-default-detach/design.md
openspec/changes/**/proposal.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
List spec deltas in
proposal.mdunder 'What Changes' section, marking breaking changes with BREAKING
Files:
openspec/changes/update-launch-modes-default-detach/proposal.md
openspec/changes/**/specs/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/changes/**/specs/**/*.md: Write spec deltas using## ADDED|MODIFIED|REMOVED|RENAMED Requirementsformat with at least one#### Scenario:per requirement in spec files
When modifying existing requirements in a MODIFIED delta, paste the full requirement block including the header and all scenarios, as the archiver will replace the entire requirement
Use ADDED for new capabilities that can stand alone; use MODIFIED for changes to existing requirement behavior, scope, or acceptance criteria; use RENAMED for name-only changes
Files:
openspec/changes/update-launch-modes-default-detach/specs/app-window/spec.md
🧠 Learnings (15)
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/tests/**/*.cpp : Test organization: Directory structure mirrors `src/`. Test files named `test_<module>.cpp`. Test cases use descriptive names and sections for organization.
Applied to files:
tests/CMakeLists.txttests/app/test_cli.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/tests/**/*.{cpp,hpp} : Focus unit tests on non-GPU logic: utility functions, configuration parsing, and pipeline graph logic
Applied to files:
tests/CMakeLists.txttests/app/test_cli.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/tests/**/*.{cpp,hpp} : Use Catch2 v3 for unit testing with test files mirroring the `src/` directory structure
Applied to files:
tests/CMakeLists.txt
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Applies to openspec/changes/**/specs/**/*.md : Use ADDED for new capabilities that can stand alone; use MODIFIED for changes to existing requirement behavior, scope, or acceptance criteria; use RENAMED for name-only changes
Applied to files:
openspec/changes/update-launch-modes-default-detach/specs/app-window/spec.mdopenspec/changes/update-launch-modes-default-detach/tasks.md
📚 Learning: 2026-01-07T07:20:57.053Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.053Z
Learning: Applies to docs/**/*.hpp : Define a lightweight `Error` struct with `ErrorCode` enum, `std::string message`, and optional `std::source_location loc` in the `goggles` namespace.
Applied to files:
src/app/application.hpp
📚 Learning: 2026-01-07T07:20:57.053Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.053Z
Learning: Applies to docs/**/*.{cpp,hpp} : Use top-level namespace `goggles`. Use module namespaces `goggles::<module>` (e.g., `goggles::capture`, `goggles::render`). Use sub-module namespaces `goggles::<module>::<submodule>`. Never use `using namespace` in headers. Prefer explicit namespace qualification or scoped `using` declarations.
Applied to files:
src/app/application.hpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/**/*.cpp : Error handling macros: Use `VK_TRY` for vk::Result early return in Vulkan code. Use `GOGGLES_TRY` for Result<T> propagation (both as statement and expression). Use `GOGGLES_MUST` for internal invariants that should never fail. Use manual pattern for destructors/cleanup functions where propagation is impossible.
Applied to files:
src/app/application.hpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/**/*.hpp : Organize namespaces as `goggles` with sub-modules like `goggles::capture`, `goggles::render`; never use `using namespace` in headers
Applied to files:
src/app/application.hpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/**/*.{cpp,hpp} : Use inter-thread communication via `goggles::util::SPSCQueue` (lock-free SPSC queue) for task parallelism
Applied to files:
src/app/application.hpp
📚 Learning: 2026-01-07T07:20:57.053Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.053Z
Learning: Applies to docs/**/*.{cpp,hpp} : Use standard spdlog severity levels: trace, debug, info, warn, error, critical. Define project-specific logging macros (`GOGGLES_LOG_TRACE`, `GOGGLES_LOG_DEBUG`, etc.) wrapping spdlog.
Applied to files:
src/app/application.hpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Main thread owns: Vulkan instance, device, swapchain lifecycle; queue submission; window events and user input; job coordination. Main thread MUST NOT: block on I/O, perform heavy computation (>1ms), allocate memory in per-frame code paths.
Applied to files:
README.mdsrc/app/main.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: The shader pipeline is a multi-pass model designed for compatibility with RetroArch shader semantics
Applied to files:
README.md
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Single-threaded render loop on main thread by default. Render backend (`goggles::render`) runs on main thread. Pipeline execution runs on main thread. Job system for non-render work on worker threads. Threading introduced only when profiling justifies it (main thread CPU consistently >8ms).
Applied to files:
README.mdsrc/app/main.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/**/*.toml : Use TOML format for all configuration files. Use `toml11` or `tomlplusplus` library. No JSON (no comments) or YAML (complex, ambiguous). Development/testing uses `config/goggles.toml` in repository root.
Applied to files:
config/goggles.toml
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Support RetroArch shader format for wide compatibility
Applied to files:
config/goggles.toml
🧬 Code graph analysis (3)
src/app/main.cpp (5)
src/app/application.cpp (4)
x11_display(112-117)x11_display(112-112)wayland_display(119-124)wayland_display(119-119)src/input/input_forwarder.cpp (4)
x11_display(216-218)x11_display(216-216)wayland_display(220-222)wayland_display(220-220)src/input/compositor_server.cpp (4)
x11_display(152-157)x11_display(152-152)wayland_display(159-161)wayland_display(159-159)src/util/error.hpp (2)
make_error(50-52)error_code_name(68-68)src/app/cli.hpp (2)
arg(76-76)parse_cli(25-25)
src/app/cli.hpp (2)
src/app/application.hpp (1)
nodiscard(46-46)src/util/error.hpp (5)
nodiscard(50-54)nodiscard(57-59)nodiscard(62-66)nodiscard(68-104)make_error(50-52)
tests/app/test_cli.cpp (1)
src/app/cli.hpp (3)
arg(76-76)nodiscard(25-149)parse_cli(25-25)
🪛 LanguageTool
openspec/changes/update-launch-modes-default-detach/proposal.md
[style] ~5-~5: To elevate your writing, try using a synonym here.
Context: ...WSI proxy sizing). This is error-prone, hard to remember, and not suitable for packa...
(HARD_TO)
⏰ 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: Build and Test (test preset)
- GitHub Check: Static Analysis (clang-tidy)
🔇 Additional comments (26)
config/goggles.toml (1)
28-28: LGTM! Shader preset updated.The shader preset change from crt-royale to zfast-crt is a straightforward configuration update. Both are valid RetroArch CRT shaders, and this change doesn't affect other configuration settings.
tests/CMakeLists.txt (1)
16-18: LGTM! Test file added following project conventions.The addition of
app/test_cli.cppcorrectly follows the project's test organization structure (mirroringsrc/directory layout) and integrates properly with the existing CMake test configuration.src/app/application.cpp (1)
112-124: LGTM! Safe accessor implementations with proper null checks.Both
x11_display()andwayland_display()correctly check for nullm_input_forwarderbefore delegating, returning an empty string when input forwarding is disabled. This is a safe and reasonable design that prevents null pointer dereferences while providing a sensible default value.src/app/application.hpp (2)
4-4: LGTM! Required header added.The
<string>header is correctly added to support the newstd::stringreturn types for the display accessor methods.
47-48: LGTM! Well-designed accessor declarations.Both accessor methods are properly marked:
[[nodiscard]]encourages callers to use the return valueconstcorrectness ensures no state modification- Return type
std::stringis appropriate for display identifiersdocs/input_forwarding.md (2)
161-162: LGTM! Documentation updated to reflect preferred workflow.The note correctly emphasizes the new default mode behavior where Goggles automatically manages the nested compositor and environment variables. This provides clear guidance for users while maintaining the manual launch instructions for reference.
179-181: LGTM! Test commands simplified for default launch mode.The updated test commands correctly demonstrate the single-command workflow (
goggles -- <test_app>), which aligns with the new default mode behavior and simplifies the testing process compared to the previous two-terminal procedure.openspec/changes/update-launch-modes-default-detach/design.md (1)
1-43: LGTM!The design document clearly defines the two launch modes, their behavior, config interaction, and hard CLI requirements. The mode definitions and dependency rules provide solid foundations for the implementation.
scripts/task/start.sh (2)
36-56: Clear usage documentation for new launch modes.The updated usage text effectively communicates the two modes (default with app launch, detach for viewer-only) and includes helpful examples. The requirement for the
--separator is clearly stated.
105-127: Simplified argument forwarding is correct.The refactored script now collects all remaining arguments and forwards them directly to the viewer binary, removing the complex environment variable and lifecycle management that has been moved to the main application. This simplification improves maintainability while maintaining correct behavior.
tests/app/test_cli.cpp (2)
11-23: Well-designed test utility.The
ArgvBuilderhelper cleanly constructsargc/argvpairs for testing, improving test readability and reducing boilerplate.
31-99: Comprehensive CLI parsing test coverage.The test suite effectively covers key scenarios:
- Detach mode validation (no app command, rejection of width/height)
- Default mode requirements (separator, app command)
- App argument isolation (options after
--don't interfere)- Width/height pairing validation
The test organization and naming align with project conventions.
README.md (2)
51-65: Clear usage documentation with practical examples.The updated usage section effectively explains the new launch modes with concrete examples. The separator requirement is clearly stated, and both default and detach modes are demonstrated. The examples progress from simple to more complex (with sizing options), which aids understanding.
67-71: Helpful lifecycle and integration notes.The note about exit behavior (viewer exits when app exits, and vice versa) clarifies the process coordination model. The Steam integration guidance is practical, with appropriate mention that full packaging support is planned.
openspec/changes/update-launch-modes-default-detach/tasks.md (1)
1-34: LGTM!The task checklist is well-organized with clear sections and appropriate completion status. The pending verification items (4.1-4.3) correctly remain unchecked as manual testing items.
openspec/changes/update-launch-modes-default-detach/specs/app-window/spec.md (1)
1-38: LGTM!The spec file correctly uses the
## MODIFIED Requirementsand## ADDED Requirementsformat with#### Scenario:headers for each requirement. The BDD-style GIVEN/WHEN/THEN structure is clear and well-organized.src/app/main.cpp (5)
17-58: LGTM!The
spawn_target_appfunction correctly handles:
- Input validation for command and display info
- Fork error handling
- Environment variable setup in child process
- Proper use of
_exit(127)after failedexecvpto avoid flushing parent's stdio buffers
69-73: LGTM!Clean helper to push a quit event to the SDL event queue for coordinated shutdown.
75-85: LGTM!Good error handling that distinguishes between help/version requests (success) and actual parsing errors (failure with guidance).
134-140: LGTM!Clear derivation of
enable_input_forwardingfrom the detach flag with informative logging when mode overrides config.
199-205: LGTM!The detach mode path correctly uses
app->run()for the simple viewer-only loop, with appropriate shutdown logging.src/app/cli.hpp (5)
13-21: LGTM!The
CliOptionsstruct is cleanly extended with the new fields for detach mode and app configuration.
28-59: LGTM!Good CLI documentation with clear usage examples, deprecation notice for
--input-forwarding, and sensible range validation (1-16384) for dimension options.
61-69: LGTM!Clean separator detection logic that correctly partitions viewer options from app command.
71-100: LGTM!The exception handling correctly distinguishes between help/version requests and actual parsing errors, with appropriate error messages for missing separator scenarios.
102-148: LGTM!Thorough post-parse validation that correctly enforces:
- Detach mode incompatibilities (
--input-forwarding,--app-width/--app-height, app command)- Default mode requirements (separator, app command)
- Width/height pair validation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/app/main.cpp (5)
31-34: Consider more specific error messaging.The error message "input forwarding display information unavailable" might be misleading. This condition occurs when the Application's display accessors return empty strings, which could happen if:
- Input forwarding failed to initialize
- The compositor server hasn't set up displays yet
- Input forwarding was disabled
Consider making the error message more specific to aid debugging, such as: "Display information not available (x11_display or wayland_display is empty)."
48-51: Validate width/height consistency.The code sets
GOGGLES_WIDTHandGOGGLES_HEIGHTonly when both are non-zero, but doesn't validate or warn when exactly one dimension is zero (e.g., width=1920, height=0). This silent acceptance of inconsistent dimensions might cause unexpected behavior.Consider validating that both dimensions are zero or both are non-zero, and return an error for invalid combinations.
🛡️ Proposed validation
- if (app_width != 0 && app_height != 0) { + if (app_width != 0 || app_height != 0) { + if (app_width == 0 || app_height == 0) { + return goggles::make_error<pid_t>(goggles::ErrorCode::invalid_config, + "app_width and app_height must both be zero or both non-zero"); + } env_overrides.emplace_back("GOGGLES_WIDTH=" + std::to_string(app_width)); env_overrides.emplace_back("GOGGLES_HEIGHT=" + std::to_string(app_height)); }
71-76: Document const_cast safety.The
const_cast<char*>on line 74 is required by the POSIXposix_spawnpAPI, which takeschar* const argv[]but doesn't modify the strings. While this is safe in practice (the strings come from the command vector and won't be modified), it's worth adding a brief comment to document this requirement and reassure future maintainers.📝 Suggested comment
std::vector<char*> argv; argv.reserve(command.size() + 1); for (const auto& arg : command) { + // const_cast required for POSIX API; posix_spawnp doesn't modify argv argv.push_back(const_cast<char*>(arg.c_str())); } argv.push_back(nullptr);
123-132: Consider checking kill() return values.The return values of
kill()calls are currently ignored. While this might be acceptable for robustness, checking forESRCH(no such process) could provide useful diagnostic information and avoid unnecessary waiting.🔍 Proposed improvement
- (void)kill(pid, SIGTERM); + if (kill(pid, SIGTERM) != 0 && errno == ESRCH) { + // Process already exited + return; + } if (reap_with_timeout(pid, SIGTERM_TIMEOUT)) { return; } GOGGLES_LOG_WARN("Target app did not exit after SIGTERM; sending SIGKILL (pid={})", pid); - (void)kill(pid, SIGKILL); + if (kill(pid, SIGKILL) != 0 && errno == ESRCH) { + // Process already exited + return; + } if (reap_with_timeout(pid, SIGKILL_TIMEOUT)) { return; }
224-240: Redundant condition check.On line 225, the check
if (enable_input_forwarding)is redundant. Sinceenable_input_forwardingis set to!cli_opts.detachon line 202, and this entire block is guarded byif (!cli_opts.detach)on line 224, the condition will always be true.Consider removing the inner condition or adding a comment explaining if there's a future scenario where these might differ.
♻️ Simplified code
if (!cli_opts.detach) { - if (enable_input_forwarding) { - const auto x11_display = app->x11_display(); - const auto wayland_display = app->wayland_display(); - - auto spawn_result = - spawn_target_app(cli_opts.app_command, x11_display, wayland_display, - cli_opts.app_width, cli_opts.app_height); - if (!spawn_result) { - GOGGLES_LOG_CRITICAL("Failed to launch target app: {} ({})", - spawn_result.error().message, - goggles::error_code_name(spawn_result.error().code)); - return EXIT_FAILURE; - } - child_pid = spawn_result.value(); - GOGGLES_LOG_INFO("Launched target app (pid={})", child_pid); + const auto x11_display = app->x11_display(); + const auto wayland_display = app->wayland_display(); + + auto spawn_result = + spawn_target_app(cli_opts.app_command, x11_display, wayland_display, + cli_opts.app_width, cli_opts.app_height); + if (!spawn_result) { + GOGGLES_LOG_CRITICAL("Failed to launch target app: {} ({})", + spawn_result.error().message, + goggles::error_code_name(spawn_result.error().code)); + return EXIT_FAILURE; } + child_pid = spawn_result.value(); + GOGGLES_LOG_INFO("Launched target app (pid={})", child_pid);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/main.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Single-threaded render loop on main thread by default. Render backend (`goggles::render`) runs on main thread. Pipeline execution runs on main thread. Job system for non-render work on worker threads. Threading introduced only when profiling justifies it (main thread CPU consistently >8ms).
Applied to files:
src/app/main.cpp
🧬 Code graph analysis (1)
src/app/main.cpp (6)
src/app/application.cpp (4)
x11_display(112-117)x11_display(112-112)wayland_display(119-124)wayland_display(119-119)src/input/input_forwarder.cpp (4)
x11_display(216-218)x11_display(216-216)wayland_display(220-222)wayland_display(220-220)src/input/compositor_server.cpp (4)
x11_display(152-157)x11_display(152-152)wayland_display(159-161)wayland_display(159-159)src/util/config.hpp (1)
to_string(19-19)src/app/cli.hpp (1)
arg(76-76)src/app/application.hpp (1)
config(29-30)
⏰ 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 (5)
src/app/main.cpp (5)
4-21: LGTM!The added includes are appropriate for the process spawning, signal handling, and child process monitoring functionality introduced in this PR.
137-141: LGTM!The SDL quit event push implementation is straightforward and appropriate for signaling viewer shutdown when the child process exits.
145-153: LGTM!The improved CLI error handling provides better user feedback with clear error messages and usage hints, which enhances the user experience.
202-208: LGTM!The input forwarding logic correctly implements the default/detach mode behavior, and the informative logging helps users understand when the mode overrides the configuration.
255-259: Verify exit status semantics when viewer closes first.When the viewer exits before the target app (e.g., user closes the window), the code terminates the child and returns
EXIT_FAILURE. This might not accurately represent all scenarios—if a user intentionally closes the viewer, should this be treated as a failure?Consider whether:
EXIT_SUCCESSmight be more appropriate for user-initiated viewer closure- A different exit code could distinguish "viewer closed first" from "child failed to launch"
- The current behavior is intentional to indicate the child was forcibly terminated
The current implementation ensures the child is cleaned up, which is correct for resource management.
1 participant
User description
Why
Launching a target app with Goggles' “full feature” workflow currently requires users to manually provide environment variables that are not stable or user-friendly (e.g., nested
DISPLAY/WAYLAND_DISPLAYvalues and WSI proxy sizing). This is error-prone, hard to remember, and not suitable for packaging/Steam launch options.What Changes
--detach): “full feature” launch mode intended for typical use and packaging.--detach): viewer-only mode intended for advanced/manual launching.GOGGLES_WSI_PROXY=1is forced for the launched app.DISPLAYandWAYLAND_DISPLAYare set for the launched app (both are provided).--app-widthand--app-heightare supported and only apply here (mapped toGOGGLES_WIDTH/GOGGLES_HEIGHT).--app-width,--app-height).--wsi-proxytoggle; WSI proxy is implicit in default mode.config/goggles.toml(and--configoverrides) for viewer behavior.Impact
app-window(CLI + launch-mode UX behavior)src/app/cli.hpp/src/app/main.cpp/src/app/application.*(mode parsing + orchestration)scripts/task/start.sh(optional: simplify to use new mode, reduce manual env usage)docs/(update user-facing launch guidance)Non-Goals
Open Questions
PR Type
Enhancement
Description
Add
--detachflag for viewer-only mode and default mode for full-feature app launchingImplement process lifecycle management with automatic app spawning and environment setup
Add
--app-widthand--app-heightCLI options for WSI proxy virtual surface sizingEnforce strict CLI option dependencies (detach mode rejects app sizing and input forwarding)
Simplify launch workflow by automatically setting
GOGGLES_CAPTURE,GOGGLES_WSI_PROXY,DISPLAY, andWAYLAND_DISPLAYDiagram Walkthrough
File Walkthrough
6 files
Add detach mode and app sizing CLI optionsImplement CLI parsing with mode validationAdd display accessor methods for app spawningImplement x11_display and wayland_display accessorsImplement app spawning and process lifecycle managementSimplify launch script to use new mode semantics1 files
Add comprehensive CLI parsing tests2 files
Register CLI tests in build systemUpdate default shader preset selection6 files
Update usage examples for new launch modesDocument preferred default mode app launchingAdd formal change proposal documentationAdd design notes for mode implementationAdd formal requirements and scenariosAdd implementation task checklistSummary by CodeRabbit
New Features
Changes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.