Skip to content

Refactor Path Resolution and Config Loading#52

Merged
K1ngst0m merged 2 commits intomainfrom
dev/filesystem
Jan 15, 2026
Merged

Refactor Path Resolution and Config Loading#52
K1ngst0m merged 2 commits intomainfrom
dev/filesystem

Conversation

@K1ngst0m
Copy link
Copy Markdown
Collaborator

@K1ngst0m K1ngst0m commented Jan 14, 2026

User description

Why

Goggles currently resolves filesystem paths (resource/config/data/cache/runtime) in multiple places,
mixing environment variables, XDG defaults, and working-directory-relative fallbacks. This can lead to
inconsistent behavior across Linux systems and packaged runs (AppImage), and makes it hard to
centralize filesystem access.

Config loading also needs a clearly defined, early validation and fallback strategy so that:

  • invalid or missing configs do not cause surprising failures in non-strict flows
  • errors are logged once at the right boundary (per docs/project_policies.md)

What Changes

  • Introduce a single goggles::util path resolution API that resolves only these directory roots:
    resource_dir, config_dir, data_dir, cache_dir, runtime_dir.
  • Add optional config-driven overrides for those five roots via [paths] in the TOML config.
  • Define a two-phase bootstrap flow:
    1. resolve config dir + config file candidate early
    2. load + validate config; on failure, fall back with a warning (unless strict/explicit config rules apply)
  • Update runtime to avoid relying on CWD for shipped assets when packaged (AppImage), consistent with
    the existing packaging spec requirement “Packaged Assets Are Not CWD-Dependent”.

Non-Goals

  • Centralizing every file open/read/write call behind a virtual filesystem interface.
  • Adding leaf-specific overrides (e.g., per-file paths). Only directory roots are in scope.
  • Changing shader/preset semantics or adding new user-facing features beyond path resolution and
    config load behavior.

Impact

  • Affected specs:
    • packaging (clarify resource root expectations and CWD independence)
    • New capability: config-loading (define requirements for config discovery/validation/fallback and
      path overrides)
  • Affected code (expected):
    • src/app/main.cpp (bootstrap + config load flow)
    • src/util/config.* (parse [paths] and expose overrides)
    • New module in src/util/ for path resolution
    • Call sites in src/ that currently resolve paths ad-hoc (follow-up refactor task)

Compatibility Notes

  • Existing config files remain valid; [paths] is optional.
  • Default behavior continues to follow XDG conventions; overrides only narrow behavior.
  • Config parse/validation failures fall back with warn logging, including when --config is
    explicitly provided (the explicit config is ignored and defaults are used).

PR Type

Enhancement


Description

  • Centralize filesystem path resolution via new goggles::util::paths API

  • Support optional [paths] config overrides for five directory roots

  • Implement two-phase bootstrap: resolve config early, load with fallback

  • Remove CWD dependency for packaged assets (AppImage compliance)

  • Refactor all path resolution call sites to use unified API


Diagram Walkthrough

flowchart LR
  CLI["CLI args<br/>--config"]
  ENV["Environment<br/>XDG vars"]
  BOOTSTRAP["Bootstrap:<br/>resolve_config_dir"]
  LOAD["Load & validate<br/>config file"]
  MERGE["Merge overrides:<br/>CLI > config > env"]
  RESOLVE["Resolve full<br/>AppDirs"]
  FALLBACK["Fallback to<br/>defaults + warn"]
  PATHS["Use unified<br/>path API"]
  
  CLI --> BOOTSTRAP
  ENV --> BOOTSTRAP
  BOOTSTRAP --> LOAD
  LOAD -->|success| MERGE
  LOAD -->|failure| FALLBACK
  MERGE --> RESOLVE
  FALLBACK --> RESOLVE
  RESOLVE --> PATHS
Loading

File Walkthrough

Relevant files
Enhancement
16 files
paths.hpp
New path resolution API with XDG compliance                           
+141/-0 
paths.cpp
Implement centralized path resolver and directory root logic
+281/-0 
config.hpp
Add Paths struct for config-driven directory overrides     
+8/-0     
config.cpp
Parse [paths] section and refactor config parsing into helpers
+149/-40
main.cpp
Implement two-phase bootstrap with early config validation
+84/-118
application.cpp
Accept AppDirs parameter and use unified path API               
+6/-18   
application.hpp
Update Application::create signature to accept AppDirs     
+3/-2     
ui_controller.cpp
Use AppDirs for font and preset catalog path resolution   
+9/-37   
ui_controller.hpp
Update UiController::create to accept AppDirs parameter   
+5/-2     
cli.hpp
Remove early config file existence check from CLI parser 
+1/-7     
vulkan_backend.hpp
Add cache_dir parameter to VulkanBackend::create                 
+2/-0     
vulkan_backend.cpp
Accept and use cache_dir for shader compilation                   
+13/-3   
shader_runtime.hpp
Update ShaderRuntime::create to accept cache directory     
+3/-1     
shader_runtime.cpp
Remove XDG path resolution; use injected cache directory 
+5/-17   
imgui_layer.hpp
Update ImGuiLayer::create to accept AppDirs parameter       
+6/-2     
imgui_layer.cpp
Use AppDirs for font and imgui.ini path resolution             
+20/-47 
Documentation
6 files
goggles.template.toml
Add commented [paths] section with override documentation
+12/-0   
proposal.md
Document change rationale and scope                                           
+45/-0   
design.md
Define path resolution API and bootstrap flow design         
+80/-0   
spec.md
Add config-loading capability specification                           
+73/-0   
spec.md
Update packaging spec for CWD-independent resource root   
+14/-0   
tasks.md
Track implementation tasks and completion status                 
+24/-0   
Tests
4 files
test_paths.cpp
Add comprehensive path resolution unit tests                         
+196/-0 
test_shader_runtime.cpp
Update tests to pass cache directory to ShaderRuntime       
+20/-11 
test_slang_reflect.cpp
Update tests to pass cache directory to ShaderRuntime       
+13/-3   
test_zfast_integration.cpp
Update tests to pass cache directory to ShaderRuntime       
+12/-2   
Configuration changes
2 files
CMakeLists.txt
Add paths.cpp to util library build                                           
+1/-0     
CMakeLists.txt
Add test_paths.cpp to test suite                                                 
+1/-0     

Summary by CodeRabbit

Release Notes

  • New Features

    • Added support for customizable application directories via optional configuration overrides.
    • Configuration file now supports path customization for resources, config, data, cache, and runtime directories.
    • Improved configuration loading with automatic fallback behavior and template-based initialization.
  • Improvements

    • Enhanced asset resolution to support packaged distributions without current working directory dependencies.
    • Better XDG Base Directory compliance with environment variable support.
  • Bug Fixes

    • Configuration validation now continues gracefully with defaults instead of failing on missing or invalid configuration.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Jan 14, 2026

📝 Walkthrough

Walkthrough

This PR refactors path resolution and configuration loading to support centralized directory management across the application. It introduces a new goggles::util::paths API with structures for path overrides and app directories, extends the Config structure with an optional [paths] section for directory overrides, updates the main bootstrap flow to resolve directories early, and cascades AppDirs through application factory methods (Application, UiController, ImGuiLayer, VulkanBackend, ShaderRuntime).

Changes

Cohort / File(s) Summary
Configuration & Overrides
config/goggles.template.toml, src/util/config.hpp, src/util/config.cpp
Added optional [paths] block to TOML template with resource_dir, config_dir, data_dir, cache_dir, runtime_dir fields. Extended Config struct with Paths nested struct. Refactored config parsing into per-section handlers with validate_absolute_or_empty checks and parse_paths extraction.
Path Resolution API
src/util/paths.hpp, src/util/paths.cpp, src/util/CMakeLists.txt
New public API for centralized path resolution: PathOverrides, ResolveContext, AppDirs, OverrideMerge structs. Functions for merge_overrides, overrides_from_config, resolve_config_dir, resolve_app_dirs, and path composition helpers (resource_path, config_path, data_path, cache_path, runtime_path). Handles XDG environment variables, absolute path validation, and override precedence.
Application Bootstrap
src/app/main.cpp, src/app/cli.hpp
Reworked config loading into load_config_for_cli with two-phase bootstrap (resolve config_dir, then load/validate config with fallback). Added get_exe_dir() resolver. Removed legacy XDG/template helpers. Integrated AppDirs resolution with preset path handling via util::resource_path. Removed config file existence check from CLI parsing.
Application Factory Updates
src/app/application.hpp, src/app/application.cpp, src/app/ui_controller.hpp, src/app/ui_controller.cpp
Application::create and UiController::create signatures now accept AppDirs parameter. Replaced hardcoded resolve_shader_base_dir() with util::resource_path/util::cache_path helpers using resolved app_dirs.
Rendering Backend & Shader Runtime
src/render/backend/vulkan_backend.hpp, src/render/backend/vulkan_backend.cpp, src/render/shader/shader_runtime.hpp, src/render/shader/shader_runtime.cpp
VulkanBackend::create and ShaderRuntime::create now accept cache_dir parameter. ShaderRuntime stores m_cache_dir and initializes cache directory at resolved path. Cache path resolution now uses m_cache_dir instead of environment variables.
UI Layer
src/ui/imgui_layer.hpp, src/ui/imgui_layer.cpp
ImGuiLayer::create signature updated to accept AppDirs parameter. Font path resolved via util::resource_path(app_dirs, "assets/fonts/..."). IMGUI ini path now uses app_dirs.config_dir. rebuild_fonts refactored to obtain ImGuiIO internally instead of accepting it as parameter.
Design & Specification
openspec/changes/refactor-path-resolution-and-config-loading/*
Added design proposal, specification docs for config-loading behavior, packaging requirements, and task/planning documentation covering path resolution rules, two-phase bootstrap, XDG handling, and error fallback strategies.
Tests
tests/util/test_paths.cpp, tests/CMakeLists.txt, tests/render/test_shader_runtime.cpp, tests/render/test_slang_reflect.cpp, tests/render/test_zfast_integration.cpp
Added comprehensive unit tests for path resolution utilities (merge_overrides, resolve_config_dir, resolve_app_dirs). Updated shader runtime tests to pass test_cache_dir() helper to ShaderRuntime::create. Added EnvVarGuard and TempDir test utilities.

Sequence Diagram(s)

sequenceDiagram
    participant CLI as CLI Parser
    participant Main as main()
    participant PathResolver as Path Resolver
    participant ConfigLoader as Config Loader
    participant AppFactory as Application Factory
    
    CLI->>Main: Parsed CliOptions
    Main->>PathResolver: resolve_config_dir(overrides)
    PathResolver-->>Main: config_dir
    Main->>ConfigLoader: load_config_for_cli(cli_opts, bootstrap_dirs)
    ConfigLoader-->>Main: Config (with [paths] overrides)
    Main->>PathResolver: extract overrides_from_config()
    PathResolver-->>Main: PathOverrides
    Main->>PathResolver: resolve_app_dirs(context, merged_overrides)
    PathResolver-->>Main: AppDirs (resource, config, data, cache, runtime)
    Main->>AppFactory: Application::create(config, app_dirs, ...)
    AppFactory->>PathResolver: util::resource_path(app_dirs, "shaders")
    PathResolver-->>AppFactory: resolved shader path
    AppFactory-->>Main: Application instance
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~65 minutes

Possibly related PRs

Suggested labels

refactor, config, paths, bootstrap, complex

Poem

🐰 Paths resolved, directories arranged,
Bootstrap flows in phases, configs changed,
AppDirs passed through every seam,
XDG-aware, a cleaner dream!
No more hardcoded, no more lost,
Where resources hide—we've crossed! 🎯

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Refactor Path Resolution and Config Loading' accurately summarizes the main changes: centralized path resolution API, config-driven overrides, and improved config loading strategy.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

🧹 Recent nitpick comments
openspec/changes/refactor-path-resolution-and-config-loading/proposal.md (1)

13-22: Consider explicitly listing spec deltas per coding guidelines.

The coding guidelines require spec deltas to be listed in proposal.md under the "What Changes" section, with breaking changes marked using BREAKING. Currently, the affected specs (packaging, config-loading) are mentioned in the Impact section (lines 31-34), but the What Changes section focuses on implementation details.

Consider restructuring to include explicit spec delta references here, e.g.:

  • packaging – MODIFIED: Packaged Assets Are Not CWD-Dependent
  • config-loading – ADDED (new capability)

As per coding guidelines for openspec/changes/**/proposal.md.

openspec/changes/refactor-path-resolution-and-config-loading/specs/packaging/spec.md (1)

9-13: Scenario step format deviates from coding guidelines.

The coding guidelines specify using - **WHEN** [condition] - **THEN** [expected result] format for scenario steps. The current format uses separate lines for each keyword (GIVEN, WHEN, THEN, AND).

Consider consolidating to match the specified format, e.g.:

#### Scenario: AppImage provides a stable resource root
- **GIVEN** the Goggles AppImage is executed from an arbitrary working directory
- **WHEN** the viewer loads its default configuration template and shader assets - **THEN** the viewer SHALL locate shipped assets via a stable `resource_dir` resolution rule **AND** it SHALL NOT require `./config` or `./shaders` to exist in the working directory

As per coding guidelines for openspec/**/*.md.

tests/render/test_slang_reflect.cpp (1)

9-16: Consider per-test cache isolation to prevent test interdependencies.

All tests share the same goggles_test_cache directory, which could cause interference if cached SPIR-V artifacts from one test affect another. For true test isolation, consider:

  1. Using a unique subdirectory per test (e.g., include test name or UUID)
  2. Clearing the cache before each test
  3. Accepting shared cache as intentional for build performance

Also, the error_code from create_directories is silently discarded—if directory creation fails, subsequent cache operations will fail with potentially confusing errors.

♻️ Optional: Per-test unique cache directories
 namespace {
-auto test_cache_dir() -> std::filesystem::path {
-    auto dir = std::filesystem::temp_directory_path() / "goggles_test_cache";
+auto test_cache_dir(const std::string& test_name) -> std::filesystem::path {
+    auto dir = std::filesystem::temp_directory_path() / "goggles_test_cache" / test_name;
     std::error_code ec;
     std::filesystem::create_directories(dir, ec);
+    if (ec) {
+        // Could use WARN() from Catch2 or just let the test fail downstream
+    }
     return dir;
 }
 } // namespace
tests/render/test_shader_runtime.cpp (1)

9-16: Consider extracting test_cache_dir() to a shared test utility.

This helper is duplicated verbatim in tests/render/test_slang_reflect.cpp and tests/render/test_zfast_integration.cpp. Consider extracting it to a shared test utility header (e.g., tests/test_helpers.hpp) to reduce duplication and ensure consistent behavior.

Additionally, using a fixed directory name (goggles_test_cache) could cause interference if tests run in parallel. Consider appending a unique identifier (e.g., test name or random suffix) for better isolation.

♻️ Suggested shared helper
// tests/test_helpers.hpp
`#pragma` once
`#include` <filesystem>

namespace goggles::test {
inline auto test_cache_dir(const std::string& suffix = "") -> std::filesystem::path {
    auto dir = std::filesystem::temp_directory_path() / "goggles_test_cache";
    if (!suffix.empty()) {
        dir /= suffix;
    }
    std::error_code ec;
    std::filesystem::create_directories(dir, ec);
    return dir;
}
} // namespace goggles::test
src/render/shader/shader_runtime.cpp (1)

188-254: Consider adding a default fallback for empty cache_dir.

Unlike VulkanBackend::create (which provides a fallback to temp_directory_path() / "goggles" / "shaders" when cache_dir is empty), ShaderRuntime::create does not handle an empty cache_dir. If an empty path is passed, create_directories will attempt to create an empty path, which may behave unexpectedly.

If the design expects callers to always provide a valid path, this is fine. Otherwise, consider adding a similar fallback for consistency:

♻️ Optional fallback pattern
 auto ShaderRuntime::create(const std::filesystem::path& cache_dir) -> ResultPtr<ShaderRuntime> {
     GOGGLES_PROFILE_FUNCTION();

     auto runtime = std::unique_ptr<ShaderRuntime>(new ShaderRuntime());
-    runtime->m_cache_dir = cache_dir;
+    if (cache_dir.empty()) {
+        try {
+            runtime->m_cache_dir = std::filesystem::temp_directory_path() / "goggles" / "shaders";
+        } catch (...) {
+            runtime->m_cache_dir = "/tmp/goggles/shaders";
+        }
+    } else {
+        runtime->m_cache_dir = cache_dir;
+    }
openspec/changes/refactor-path-resolution-and-config-loading/specs/config-loading/spec.md (2)

9-13: Scenario step format differs from coding guidelines.

The coding guidelines specify scenario steps should use the format - **WHEN** [condition] - **THEN** [expected result] (inline on a single line). The current format uses separate bullet points for GIVEN/WHEN/THEN. Consider consolidating to match the prescribed format.

📝 Suggested format adjustment
 #### Scenario: Default config path is resolved
-- **GIVEN** no explicit `--config` argument is provided
-- **WHEN** Goggles starts
-- **THEN** it SHALL resolve the default config path under XDG config home
+- **WHEN** Goggles starts without an explicit `--config` argument - **THEN** it SHALL resolve the default config path under XDG config home

23-28: Multi-step scenario could consolidate THEN clauses.

When a scenario has multiple THEN assertions, consider using AND inline or consolidating into a single expected result statement per the coding guidelines format.

📝 Suggested format
 #### Scenario: Missing config falls back to defaults
-- **GIVEN** no config file exists at the resolved default path
-- **WHEN** Goggles starts
-- **THEN** it SHALL log a warning
-- **AND** it SHALL continue using default configuration values
+- **WHEN** no config file exists at the resolved default path and Goggles starts - **THEN** it SHALL log a warning and continue using default configuration values
src/render/backend/vulkan_backend.cpp (1)

189-196: Fallback logic is reasonable but consider logging when using fallback path.

The fallback to temp_directory_path() with /tmp/goggles/shaders as ultimate fallback handles edge cases well. However, when the fallback is triggered (either due to empty input or exception), it might be helpful to log a debug message so operators know the cache location being used.

♻️ Optional: Add debug logging for fallback cache path
     backend->m_cache_dir = cache_dir;
     if (backend->m_cache_dir.empty()) {
         try {
             backend->m_cache_dir = std::filesystem::temp_directory_path() / "goggles" / "shaders";
         } catch (...) {
             backend->m_cache_dir = "/tmp/goggles/shaders";
         }
+        GOGGLES_LOG_DEBUG("Using fallback shader cache path: {}", backend->m_cache_dir.string());
     }
src/util/paths.hpp (1)

37-46: Consider documenting the resolved state.

The struct could benefit from a brief note indicating that these paths are guaranteed to be resolved and usable (unlike PathOverrides which may be empty). This would clarify the contract for consumers.


📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 376ca8a and e014475.

📒 Files selected for processing (28)
  • config/goggles.template.toml
  • openspec/changes/refactor-path-resolution-and-config-loading/design.md
  • openspec/changes/refactor-path-resolution-and-config-loading/proposal.md
  • openspec/changes/refactor-path-resolution-and-config-loading/specs/config-loading/spec.md
  • openspec/changes/refactor-path-resolution-and-config-loading/specs/packaging/spec.md
  • openspec/changes/refactor-path-resolution-and-config-loading/tasks.md
  • src/app/application.cpp
  • src/app/application.hpp
  • src/app/cli.hpp
  • src/app/main.cpp
  • src/app/ui_controller.cpp
  • src/app/ui_controller.hpp
  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
  • src/render/shader/shader_runtime.cpp
  • src/render/shader/shader_runtime.hpp
  • src/ui/imgui_layer.cpp
  • src/ui/imgui_layer.hpp
  • src/util/CMakeLists.txt
  • src/util/config.cpp
  • src/util/config.hpp
  • src/util/paths.cpp
  • src/util/paths.hpp
  • tests/CMakeLists.txt
  • tests/render/test_shader_runtime.cpp
  • tests/render/test_slang_reflect.cpp
  • tests/render/test_zfast_integration.cpp
  • tests/util/test_paths.cpp
🧰 Additional context used
📓 Path-based instructions (3)
openspec/changes/**/specs/**/*.md

📄 CodeRabbit inference engine (openspec/AGENTS.md)

openspec/changes/**/specs/**/*.md: Write spec deltas using ## ADDED|MODIFIED|REMOVED|RENAMED Requirements format 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/refactor-path-resolution-and-config-loading/specs/packaging/spec.md
  • openspec/changes/refactor-path-resolution-and-config-loading/specs/config-loading/spec.md
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/refactor-path-resolution-and-config-loading/specs/packaging/spec.md
  • openspec/changes/refactor-path-resolution-and-config-loading/proposal.md
  • openspec/changes/refactor-path-resolution-and-config-loading/design.md
  • openspec/changes/refactor-path-resolution-and-config-loading/specs/config-loading/spec.md
  • openspec/changes/refactor-path-resolution-and-config-loading/tasks.md
openspec/changes/**/proposal.md

📄 CodeRabbit inference engine (openspec/AGENTS.md)

List spec deltas in proposal.md under 'What Changes' section, marking breaking changes with BREAKING

Files:

  • openspec/changes/refactor-path-resolution-and-config-loading/proposal.md
🧠 Learnings (42)
📚 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.txt
  • tests/render/test_zfast_integration.cpp
  • src/util/CMakeLists.txt
  • tests/render/test_shader_runtime.cpp
  • tests/render/test_slang_reflect.cpp
  • tests/util/test_paths.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
  • tests/render/test_zfast_integration.cpp
  • tests/render/test_shader_runtime.cpp
  • tests/render/test_slang_reflect.cpp
  • tests/util/test_paths.cpp
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
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.txt
  • tests/render/test_shader_runtime.cpp
  • tests/render/test_slang_reflect.cpp
  • tests/util/test_paths.cpp
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
Learning: Applies to docs/tests/**/*.cpp : Use Catch2 v3 as the project-wide testing framework. Header-only option, provided by Pixi environment, loaded via `find_package`.

Applied to files:

  • tests/CMakeLists.txt
  • tests/render/test_zfast_integration.cpp
  • tests/util/test_paths.cpp
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
Learning: Testing scope: Only non-GPU logic tested initially. In scope: utility functions, configuration parsing, error handling, pipeline graph logic. Out of scope: Vulkan initialization, GPU resource management, rendering, capture layer.

Applied to files:

  • tests/CMakeLists.txt
  • openspec/changes/refactor-path-resolution-and-config-loading/tasks.md
  • tests/render/test_shader_runtime.cpp
  • src/render/backend/vulkan_backend.cpp
  • src/util/config.cpp
📚 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: Place spec delta files under `changes/[change-id]/specs/<capability>/spec.md`, creating one delta file per affected capability

Applied to files:

  • openspec/changes/refactor-path-resolution-and-config-loading/specs/packaging/spec.md
  • openspec/changes/refactor-path-resolution-and-config-loading/tasks.md
📚 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 : Write spec deltas using `## ADDED|MODIFIED|REMOVED|RENAMED Requirements` format with at least one `#### Scenario:` per requirement in spec files

Applied to files:

  • openspec/changes/refactor-path-resolution-and-config-loading/specs/packaging/spec.md
  • openspec/changes/refactor-path-resolution-and-config-loading/tasks.md
📚 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 : 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

Applied to files:

  • openspec/changes/refactor-path-resolution-and-config-loading/specs/packaging/spec.md
📚 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/refactor-path-resolution-and-config-loading/specs/packaging/spec.md
  • openspec/changes/refactor-path-resolution-and-config-loading/specs/config-loading/spec.md
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
Learning: Applies to docs/**/{pixi.toml,pixi.lock} : Use Pixi as the single source of truth for dependencies. Use `pixi.toml` + `pixi.lock` for toolchains and third-party libraries. Local packages pulled into Pixi environment by path for pinned/forked builds. System packages acceptable only when not in Pixi and not practical to vendor; must be documented.

Applied to files:

  • openspec/changes/refactor-path-resolution-and-config-loading/specs/packaging/spec.md
  • openspec/changes/refactor-path-resolution-and-config-loading/tasks.md
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
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.template.toml
  • openspec/changes/refactor-path-resolution-and-config-loading/proposal.md
  • openspec/changes/refactor-path-resolution-and-config-loading/specs/config-loading/spec.md
  • openspec/changes/refactor-path-resolution-and-config-loading/tasks.md
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
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:

  • config/goggles.template.toml
  • src/app/ui_controller.hpp
  • src/util/CMakeLists.txt
  • src/util/paths.cpp
  • tests/util/test_paths.cpp
  • src/app/application.hpp
  • src/app/main.cpp
  • src/util/paths.hpp
  • src/util/config.cpp
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
Learning: Applies to docs/**/*.cpp : Configuration loading: Read once at startup (no hot-reload initially). Validate all values after parsing. Provide defaults for all optional values. Fail fast on invalid config (do not silently ignore).

Applied to files:

  • src/app/cli.hpp
  • openspec/changes/refactor-path-resolution-and-config-loading/design.md
  • openspec/changes/refactor-path-resolution-and-config-loading/specs/config-loading/spec.md
  • openspec/changes/refactor-path-resolution-and-config-loading/tasks.md
  • src/app/main.cpp
  • src/util/config.cpp
📚 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: Create a `design.md` file only when the change is cross-cutting (multiple services/modules), introduces new external dependencies, has security/performance/migration complexity, or has ambiguity requiring technical decisions

Applied to files:

  • openspec/changes/refactor-path-resolution-and-config-loading/design.md
  • openspec/changes/refactor-path-resolution-and-config-loading/tasks.md
📚 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/docs/**/*.md : Document architecture decisions, component boundaries, data flow, performance constraints, and external API contracts

Applied to files:

  • openspec/changes/refactor-path-resolution-and-config-loading/design.md
  • openspec/changes/refactor-path-resolution-and-config-loading/specs/config-loading/spec.md
  • openspec/changes/refactor-path-resolution-and-config-loading/tasks.md
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Vulkan resource management: Follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types. Call `device.waitIdle()` or wait on fences before destroying GPU-async resources. Store creation info with resources for debugging/recreation. Never leak Vulkan objects. Member ordering: declare in reverse destruction order (device before instance).

Applied to files:

  • src/render/backend/vulkan_backend.hpp
  • src/app/application.cpp
  • src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
Learning: Applies to docs/**/*.{cpp,hpp} : Object initialization policy: Use factory pattern `static auto create(...) -> ResultPtr<T>` for: (1) Manager/singleton objects (VulkanBackend, FilterChain), (2) Objects owning multiple interdependent resources, (3) Objects where uninitialized state causes undefined behavior. Banned pattern: No two-phase initialization (no constructor + `init()`).

Applied to files:

  • src/render/backend/vulkan_backend.hpp
  • src/app/application.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/**/*.toml : All configuration MUST be in TOML format

Applied to files:

  • openspec/changes/refactor-path-resolution-and-config-loading/specs/config-loading/spec.md
📚 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/ui_controller.hpp
  • src/util/CMakeLists.txt
  • src/app/application.hpp
  • src/util/paths.hpp
  • src/util/config.cpp
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
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/ui_controller.hpp
  • src/app/application.hpp
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
Learning: All builds and tests MUST use CMake presets. Never run `cmake -B build` or `cmake --build build` directly without a preset. Use `cmake --preset <name>`, `cmake --build --preset <name>`, `ctest --preset <name>`. Available presets: asan, ubsan, debug, release, relwithdebinfo.

Applied to files:

  • tests/render/test_zfast_integration.cpp
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
Learning: Applies to docs/src/render/**/*.cpp : Per-frame code paths on main thread MUST NOT: perform dynamic memory allocations (`new`, `malloc`, `std::make_shared`); use blocking synchronization primitives (`std::mutex`, `std::condition_variable`); exceed 8ms CPU time budget (excluding GPU sync).

Applied to files:

  • tests/render/test_zfast_integration.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:

  • tests/render/test_zfast_integration.cpp
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
Learning: Applies to docs/**/*.cpp : All concurrent processing MUST use the project's central job system (`goggles::util::JobSystem`). Phase 1: BS::thread_pool. Phase 2: Taskflow. Direct use of `std::thread` or `std::jthread` for pipeline work is PROHIBITED. Exception: External integration code (networking, IPC) outside real-time path may use `std::jthread` with RAII.

Applied to files:

  • src/util/CMakeLists.txt
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
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/util/CMakeLists.txt
  • src/app/application.hpp
  • src/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: 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/util/CMakeLists.txt
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
Learning: Applies to docs/**/*.cpp : Use `spdlog` as the project-wide logging library. No `std::cout`, `std::cerr`, or `printf` for subsystem logging (only for main app output).

Applied to files:

  • src/util/CMakeLists.txt
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
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/util/CMakeLists.txt
  • src/app/application.cpp
📚 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: Create a change proposal with `proposal.md`, `tasks.md`, and optional `design.md` under `openspec/changes/<change-id>/` before implementing changes to add features, make breaking changes, or update architecture

Applied to files:

  • openspec/changes/refactor-path-resolution-and-config-loading/tasks.md
📚 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/**/proposal.md : List spec deltas in `proposal.md` under 'What Changes' section, marking breaking changes with **BREAKING**

Applied to files:

  • openspec/changes/refactor-path-resolution-and-config-loading/tasks.md
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
Learning: Applies to docs/**/{pixi.toml,pixi.lock} : All dependencies must have explicit versions. Pin versions in `pixi.toml` and commit `pixi.lock`. Pin local packages by repository state or explicit version. Document rationale for version changes in commit messages.

Applied to files:

  • openspec/changes/refactor-path-resolution-and-config-loading/tasks.md
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp (C++ bindings), NOT raw Vulkan C API. Use `vk::` types (e.g., `vk::Instance`, `vk::Device`). Do not use raw C handles like `VkInstance`, `VkDevice`. Required configuration: `#define VULKAN_HPP_NO_EXCEPTIONS`, `#define VULKAN_HPP_DISPATCH_LOADER_DYNAMIC 1`, `#include <vulkan/vulkan.hpp>`.

Applied to files:

  • src/app/application.cpp
  • src/render/backend/vulkan_backend.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/src/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp with `VULKAN_HPP_NO_EXCEPTIONS` and dynamic dispatch

Applied to files:

  • src/app/application.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/src/render/**/*.{cpp,hpp} : Use `vk::Unique*` for long-lived Vulkan resources; use plain handles for per-frame/GPU-async resources

Applied to files:

  • src/app/application.cpp
  • src/render/backend/vulkan_backend.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/src/capture/vk_layer/**/*.{cpp,hpp} : Capture layer (vk_layer/) MUST use raw Vulkan C API, not vulkan-hpp

Applied to files:

  • src/app/application.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/src/capture/vk_layer/**/*.{cpp,hpp} : Do not perform blocking operations or file I/O in capture layer performance-critical paths (e.g., vkQueuePresentKHR)

Applied to files:

  • src/app/application.cpp
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Capture layer exception: The Vulkan capture layer (`src/capture/vk_layer/`) is exempt and MUST use the raw Vulkan C API because layer dispatch tables require C function pointers and the layer must intercept C API calls.

Applied to files:

  • src/app/application.cpp
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
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:

  • src/app/application.cpp
  • src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer threading: Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR` hot path. Use atomics or locks for thread-safe layer state.

Applied to files:

  • src/app/application.cpp
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer logging constraints: Use only `error` and `critical` levels. No file I/O or blocking operations in hot paths. Prefix all logs with `[goggles_vklayer]`. Never log in `vkQueuePresentKHR` hot path.

Applied to files:

  • src/app/application.cpp
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
Learning: Applies to docs/**/*.cpp : Initialize logger once at application startup for console output (development) or file output (optional). For capture layer, initialize logger lazily on first `vkCreateInstance` hook.

Applied to files:

  • src/app/application.cpp
📚 Learning: 2026-01-07T07:20:57.073Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.073Z
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
🧬 Code graph analysis (12)
src/render/backend/vulkan_backend.hpp (1)
src/render/shader/shader_runtime.hpp (1)
  • cache_dir (30-31)
src/app/ui_controller.hpp (1)
src/app/application.hpp (1)
  • config (32-33)
tests/render/test_zfast_integration.cpp (2)
tests/render/test_shader_runtime.cpp (2)
  • test_cache_dir (10-15)
  • test_cache_dir (10-10)
tests/render/test_slang_reflect.cpp (2)
  • test_cache_dir (10-15)
  • test_cache_dir (10-10)
src/ui/imgui_layer.hpp (2)
src/app/application.hpp (2)
  • nodiscard (49-49)
  • config (32-33)
src/app/ui_controller.hpp (1)
  • window (29-31)
src/render/shader/shader_runtime.cpp (2)
src/render/backend/vulkan_backend.cpp (2)
  • create (170-222)
  • create (170-173)
src/render/shader/shader_runtime.hpp (1)
  • cache_dir (30-31)
src/app/application.cpp (4)
src/app/ui_controller.cpp (2)
  • create (75-135)
  • create (75-77)
src/render/backend/vulkan_backend.cpp (2)
  • create (170-222)
  • create (170-173)
src/app/application.hpp (1)
  • config (32-33)
src/util/paths.hpp (2)
  • resource_path (98-99)
  • cache_path (128-129)
src/ui/imgui_layer.cpp (3)
src/ui/imgui_layer.hpp (1)
  • window (61-62)
src/util/paths.cpp (2)
  • resource_path (261-263)
  • resource_path (261-261)
src/util/paths.hpp (1)
  • resource_path (98-99)
src/util/paths.cpp (4)
src/app/application.hpp (1)
  • config (32-33)
src/util/error.hpp (1)
  • make_error (50-52)
src/util/paths.hpp (1)
  • resolve_config_dir (78-79)
src/render/shader/shader_runtime.hpp (1)
  • cache_dir (30-31)
tests/util/test_paths.cpp (1)
src/util/paths.cpp (10)
  • merge_overrides (135-155)
  • merge_overrides (135-135)
  • resolve_config_dir (179-196)
  • resolve_config_dir (179-179)
  • resolve_app_dirs (198-259)
  • resolve_app_dirs (198-199)
  • resource_path (261-263)
  • resource_path (261-261)
  • config_path (265-267)
  • config_path (265-265)
src/app/ui_controller.cpp (3)
src/app/application.hpp (1)
  • config (32-33)
src/util/paths.cpp (4)
  • data_path (269-271)
  • data_path (269-269)
  • resource_path (261-263)
  • resource_path (261-261)
src/util/paths.hpp (2)
  • data_path (118-119)
  • resource_path (98-99)
src/app/main.cpp (3)
tests/app/test_cli.cpp (1)
  • default_config_path (25-25)
src/util/paths.hpp (2)
  • config_path (108-109)
  • resource_path (98-99)
src/util/config.hpp (2)
  • load_config (71-71)
  • default_config (72-72)
src/util/config.cpp (4)
src/render/chain/preset_parser.hpp (2)
  • value (72-72)
  • value (73-73)
src/util/error.hpp (1)
  • make_error (50-52)
src/util/config.hpp (2)
  • to_string (19-19)
  • default_config (72-72)
src/ui/imgui_layer.hpp (2)
  • path (79-79)
  • path (106-106)
🪛 Cppcheck (2.19.0)
src/render/shader/shader_runtime.cpp

[error] 249-249: syntax error

(syntaxError)

🪛 LanguageTool
openspec/changes/refactor-path-resolution-and-config-loading/proposal.md

[style] ~6-~6: To elevate your writing, try using a synonym here.
Context: ... packaged runs (AppImage), and makes it hard to centralize filesystem access. Confi...

(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: Static Analysis (clang-tidy)
  • GitHub Check: Build and Test (test preset)
🔇 Additional comments (69)
openspec/changes/refactor-path-resolution-and-config-loading/proposal.md (1)

1-45: Overall structure and content look good.

The proposal clearly articulates the problem, proposed solution, non-goals, impact, and compatibility considerations. The two-phase bootstrap flow and directory root abstraction are well-motivated.

src/ui/imgui_layer.hpp (2)

17-19: Forward declaration is appropriate.

Using a forward declaration for goggles::util::AppDirs is the right approach since only a const reference is used in the interface. This minimizes header dependencies.


61-62: API extension is consistent with the broader refactor.

The addition of const util::AppDirs& app_dirs parameter aligns with the corresponding changes in UiController::create and Application::create (per relevant snippets), maintaining a consistent pattern for propagating directory configuration through the creation flow.

tests/CMakeLists.txt (1)

27-27: Test registration follows project conventions.

Adding util/test_paths.cpp correctly mirrors the src/ directory structure and aligns with the project policy of focusing unit tests on non-GPU utility functions. Based on learnings, this is the appropriate approach for testing the new path resolution utilities.

src/render/backend/vulkan_backend.hpp (2)

159-161: Member placement follows good organization.

The m_cache_dir member is positioned alongside related path members (m_shader_dir, m_preset_path), which aids readability and maintains logical grouping.


31-34: Cache directory parameter addition is well-integrated with proper fallback handling.

The cache_dir parameter placement between shader_dir and settings maintains logical grouping of path-related parameters. The default empty path {} preserves backward compatibility for existing call sites. Empty cache directories gracefully fall back to a system temp directory (std::filesystem::temp_directory_path() / "goggles" / "shaders"), with additional fallback to /tmp/goggles/shaders if temp directory access fails.

openspec/changes/refactor-path-resolution-and-config-loading/specs/packaging/spec.md (1)

1-6: Spec delta structure is correct.

The file correctly uses ## MODIFIED Requirements format with a clear requirement header. The requirement text clearly specifies the expected behavior for CWD-independent asset resolution.

config/goggles.template.toml (1)

8-18: LGTM! Well-documented path overrides section.

The commented-out [paths] template provides clear inline documentation for each override. The XDG defaults and absolute path requirement are appropriately noted. Based on learnings, TOML format aligns with project configuration policy.

src/util/CMakeLists.txt (1)

4-9: LGTM!

The paths.cpp addition fits naturally into the util library. Existing dependencies (toml11, spdlog) should cover path resolution needs.

src/util/config.hpp (1)

36-42: LGTM! Clean config extension for path overrides.

Using std::string is appropriate here since TOML parsing yields strings and conversion to std::filesystem::path should occur during resolution. Empty defaults correctly represent "use XDG fallbacks."

src/render/shader/shader_runtime.hpp (2)

30-31: LGTM! Explicit cache directory injection improves testability and path control.

The API change from implicit cache resolution to explicit cache_dir parameter is a good design decision—it enables proper dependency injection for tests and ensures the ShaderRuntime doesn't rely on global state or CWD assumptions.


81-81: Appropriate storage for injected cache directory.

Storing by value (std::filesystem::path m_cache_dir) is correct for owned state that the runtime will use throughout its lifetime.

tests/render/test_slang_reflect.cpp (3)

18-20: Test correctly uses the new ShaderRuntime::create API.

The test case properly passes test_cache_dir() to the factory method, aligning with the updated API signature. Based on learnings, test structure follows project policy with descriptive names and proper Catch2 usage.


55-57: LGTM!

Consistent usage of the helper across test cases.


100-102: LGTM!

Consistent pattern maintained across all three test cases.

src/app/cli.hpp (1)

41-41: LGTM - config path validation deferred to bootstrap phase.

The removal of the CLI::ExistingFile check here is intentional per the PR's two-phase bootstrap design. Config path existence and validation will be handled later with proper fallback and warning logging, which aligns with the centralized path resolution architecture.

tests/render/test_shader_runtime.cpp (1)

18-30: LGTM - test updates correctly pass cache directory.

The test sections properly create ShaderRuntime instances with the cache directory parameter, aligning with the updated API signature.

src/render/shader/shader_runtime.cpp (1)

300-302: LGTM - getter simplified to return stored cache directory.

Returning m_cache_dir directly is cleaner and aligns with the design of resolving paths at creation time rather than on each access.

tests/render/test_zfast_integration.cpp (1)

107-108: LGTM - integration test correctly updated for new API.

The ShaderRuntime::create call properly passes a cache directory, maintaining the integration test's ability to verify the full shader compilation pipeline.

openspec/changes/refactor-path-resolution-and-config-loading/tasks.md (1)

1-24: LGTM - task tracking document is well-organized.

The task list comprehensively covers the refactor scope across spec/design, implementation, testing, and documentation. All tasks are marked complete, which aligns with the code changes in this PR.

src/app/application.hpp (1)

8-8: LGTM!

The API change to accept const util::AppDirs& app_dirs aligns well with the PR's centralized path resolution design. The include placement is appropriate, and the signature change is consistent with the pattern used across other factory methods in this PR (UiController, ImGuiLayer).

Also applies to: 32-33

openspec/changes/refactor-path-resolution-and-config-loading/design.md (1)

1-80: Well-structured design document.

The design clearly articulates the path resolution API, XDG compliance rules, AppImage-friendly resource handling (avoiding CWD dependency), and the two-phase bootstrap sequence. The policy compliance section correctly enforces Result<T> returns and boundary-level logging, aligning with project error-handling conventions.

src/app/ui_controller.cpp (2)

75-77: LGTM!

The signature change correctly propagates app_dirs through the creation chain, maintaining consistency with the broader path resolution refactor.


104-108: Good fallback strategy for preset directory resolution.

The logic correctly prioritizes user-installed shaders in data_dir over bundled resources in resource_dir, aligning with XDG conventions. The error code check on line 106 properly handles both missing directory and filesystem error cases.

src/app/ui_controller.hpp (2)

10-12: Good use of forward declaration.

Using a forward declaration for util::AppDirs rather than including the full header reduces compile-time dependencies, which is appropriate since only a const reference is needed in the function signature.


29-31: LGTM!

The updated signature is consistent with the Application::create pattern and the broader AppDirs-based path resolution refactor.

openspec/changes/refactor-path-resolution-and-config-loading/specs/config-loading/spec.md (1)

1-73: Overall spec content is solid.

The requirements comprehensively cover:

  • XDG-compliant config discovery
  • Early validation with graceful fallback (covering missing, invalid, and explicit invalid cases)
  • Optional template-based bootstrap with atomic writes
  • Directory root overrides via [paths] table

This aligns well with the design document and PR objectives. The format suggestions above are optional refinements.

tests/util/test_paths.cpp (7)

16-47: LGTM! Well-designed RAII guard for environment variables.

The EnvVarGuard correctly saves, sets, and restores environment variables. Properly deleted copy/move operations prevent misuse.


49-86: LGTM with minor note on fallback behavior.

The TempDir helper is well-structured. The fallback to a non-unique directory name (line 63) when mkdtemp fails could cause test collisions if run concurrently, but this is acceptable since mkdtemp failures are rare in practice.


88-97: LGTM! Helper correctly creates the resource root sentinel structure.

This aligns with the is_resource_root() check in src/util/paths.cpp (lines 88-97) which looks for config/goggles.template.toml and shaders/ directory.


101-112: LGTM! Good coverage of override merging behavior.

The test correctly validates that high-priority values override low-priority ones while preserving fallback values.


135-167: LGTM! Comprehensive test for full app directory resolution.

This test thoroughly validates the sentinel-based resource root discovery and XDG-based directory resolution, properly isolating environment variables to ensure deterministic behavior.


169-181: LGTM! Properly tests rejection of relative path overrides.

Validates the error handling path when non-absolute paths are provided as overrides.


183-195: LGTM! Validates path normalization in join helpers.

Good test coverage for the resource_path and config_path helpers ensuring they correctly normalize paths with .. and . components.

src/app/application.cpp (2)

29-30: LGTM! API updated to accept centralized AppDirs.

The signature change properly threads app_dirs through the initialization chain, enabling centralized path resolution.


51-58: LGTM! Shader and cache paths resolved via AppDirs utilities.

The changes correctly use util::resource_path(app_dirs, "shaders") for shader sources and util::cache_path(app_dirs, "shaders") for shader cache, aligning with the centralized path resolution design. The app_dirs is also properly forwarded to UiController::create.

src/render/backend/vulkan_backend.cpp (3)

170-172: LGTM! API extended to accept cache_dir for shader caching.

The signature change aligns with the centralized path resolution design, allowing the caller to specify where shader cache files should be stored.


883-894: LGTM! ShaderRuntime correctly initialized with cache_dir.

The ShaderRuntime::create call properly uses the resolved m_cache_dir member.


1531-1543: LGTM! Async shader reload correctly captures cache_dir.

The cache_dir is captured by value for the async task, ensuring the correct cache location is used when creating the ShaderRuntime on the worker thread. This is safe since std::filesystem::path is copyable.

src/ui/imgui_layer.cpp (3)

41-68: LGTM! Simplified rebuild_fonts signature.

The function now takes the font path directly and retrieves ImGuiIO internally, reducing coupling and making the API cleaner.


72-83: LGTM! Font path resolved via centralized AppDirs utilities.

The create signature properly accepts app_dirs and uses util::resource_path to resolve the font path, ensuring consistent path resolution across the application.


91-103: Good improvement: INI file written to config_dir instead of CWD.

This change prevents imgui.ini from being written to the current working directory, which is important for packaged deployments (AppImage). Setting IniFilename = nullptr as fallback is the correct approach to disable INI persistence when the config directory can't be created.

src/util/paths.cpp (7)

11-35: LGTM! Well-designed environment variable helpers.

The helpers correctly handle empty strings as equivalent to unset, and get_env_path properly validates that paths are absolute before returning them.


37-86: LGTM! XDG directory resolution follows the spec correctly.

The implementation properly checks environment variables first and falls back to XDG defaults (~/.config, ~/.local/share, ~/.cache). The resolve_runtime_root function handles the edge case where temp_directory_path() might throw by falling back to /tmp.


88-131: LGTM! Resource root detection with sensible search order.

The search order (environment override → APPDIR → exe_dir ancestry → CWD) aligns with the PR objectives for supporting both development and packaged (AppImage) deployments. The depth limit of 8 prevents excessive filesystem traversal.


135-155: LGTM! Override merging correctly implements high-priority-wins semantics.

Empty strings in high correctly fall through to low values.


179-196: LGTM! resolve_config_dir with proper validation and fallback.

The function correctly rejects relative paths, uses explicit overrides when provided, and falls back to XDG config directory otherwise.


198-259: LGTM! Comprehensive app directory resolution with consistent validation.

The implementation validates all overrides upfront, then resolves each directory with appropriate fallbacks. The use of GOGGLES_TRY for resolve_config_dir properly propagates errors.


261-279: LGTM! Path join helpers provide consistent normalization.

Each helper correctly composes the base directory with the relative path and applies lexically_normal() for consistent output.

src/util/config.cpp (9)

9-19: LGTM!

Clean validation helper with clear semantics: empty strings pass (use defaults), non-empty must be absolute paths.


21-55: LGTM!

Per-section parser with consistent optional field handling and validation. Exception handling properly wraps TOML parsing errors.


57-79: LGTM!

Backend validation with appropriate enum allowlist. Optional field handling is consistent with other parsers.


81-95: LGTM!

Simple parser for the input section with consistent error handling.


97-111: LGTM!

Straightforward parser for shader preset configuration.


113-167: LGTM!

Comprehensive render section parser with thorough range and enum validation. Safe casts after bounds checking.


169-198: LGTM!

Log level validation is comprehensive with all spdlog severity levels covered.


202-204: LGTM!

Clean default configuration provider relying on struct member defaults.


206-236: LGTM!

Well-structured config loading with proper error distinction between stat failures and missing files. Per-section parsing with GOGGLES_TRY ensures fail-fast behavior on invalid configuration. Based on learnings, this aligns with the project policy to validate all values after parsing and fail fast on invalid config.

src/app/main.cpp (6)

27-35: LGTM!

Clean executable directory resolution using /proc/self/exe. Silent failure with empty path is appropriate as callers handle the fallback.


37-43: LGTM!

Sensible fallback strategy: packaged runs use sibling binary, development uses PATH lookup.


213-266: LGTM!

Robust configuration loading with clear fallback behavior. Template copying for first-run UX, graceful degradation with warnings when config is invalid or missing. The fallback-with-warning approach aligns with the PR objectives for improved UX over strict fail-fast.


312-343: LGTM!

Two-phase bootstrap approach correctly implements the PR spec: resolve bootstrap dirs first for config loading, then re-resolve with config-driven overrides. Safe current_path() retrieval with exception handling, and appropriate fallback if final resolution fails.


353-359: LGTM!

Clean integration of the new resource_path helper for relative preset resolution.


373-373: LGTM!

Updated call site correctly passes app_dirs to the refactored Application::create signature.

src/util/paths.hpp (5)

1-9: LGTM!

Clean header setup with minimal includes and appropriate forward declaration to avoid circular dependencies.


13-24: LGTM!

Well-documented struct with clear contract: empty uses defaults, non-empty must be absolute. Covers all five directory roots per PR objectives.


26-35: LGTM!

Clean context struct supporting both packaged (AppImage) and development workflows as described in PR objectives.


48-54: LGTM!

Good API design using a named struct to prevent parameter ordering bugs in merge operations.


56-139: LGTM!

Clean API surface with consistent [[nodiscard]] usage and well-documented contracts. The fallible operations return Result<T> while join helpers are infallible. Namespace usage follows project conventions.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

- Introduce `goggles::util` path resolver for five core directory roots
- Support optional [paths] section in config for root overrides
- Add early config resolution and validation with fallback-on-error strategy
- Tests cover XDG defaults, TOML overrides, and packaged asset independence
@K1ngst0m K1ngst0m marked this pull request as ready for review January 14, 2026 23:50
@qodo-code-review
Copy link
Copy Markdown

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: 🏷️
Unvalidated CWD fallback: resolve_app_dirs() can fall back to using ctx.cwd as resource_dir even when it does not
pass the sentinel is_resource_root() check, which can silently select an incorrect
resource root and lead to hard-to-diagnose runtime failures.

Referred Code
std::filesystem::path resource_dir;
if (!overrides.resource_dir.empty()) {
    resource_dir = overrides.resource_dir.lexically_normal();
} else if (auto found = find_resource_root(ctx)) {
    resource_dir = *found;
} else if (!ctx.cwd.empty()) {
    resource_dir = ctx.cwd.lexically_normal();
} else {
    return make_error<AppDirs>(ErrorCode::file_not_found, "Unable to resolve resource_dir");
}

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Simplify the two-phase path resolution

Simplify the two-phase path resolution in main.cpp. Instead of two full calls to
resolve_app_dirs, resolve only the config directory first, load the config, then
resolve all directories once with overrides.

Examples:

src/app/main.cpp [323-343]
    auto bootstrap_dirs_result = goggles::util::resolve_app_dirs(resolve_ctx, {});
    if (!bootstrap_dirs_result) {
        GOGGLES_LOG_WARN("Failed to resolve app directories: {} ({})",
                         bootstrap_dirs_result.error().message,
                         goggles::error_code_name(bootstrap_dirs_result.error().code));
        return EXIT_FAILURE;
    }
    const auto& bootstrap_dirs = bootstrap_dirs_result.value();

    goggles::Config config = load_config_for_cli(cli_opts, bootstrap_dirs);

 ... (clipped 11 lines)

Solution Walkthrough:

Before:

// in main.cpp
// Phase 1: Resolve all directories with default settings
auto bootstrap_dirs_result = goggles::util::resolve_app_dirs(resolve_ctx, {});
const auto& bootstrap_dirs = bootstrap_dirs_result.value();

// Load config using bootstrap directories
goggles::Config config = load_config_for_cli(cli_opts, bootstrap_dirs);

// Phase 2: Re-resolve all directories with config overrides
auto final_overrides = goggles::util::overrides_from_config(config);
auto final_dirs_result = goggles::util::resolve_app_dirs(resolve_ctx, final_overrides);

goggles::util::AppDirs app_dirs = bootstrap_dirs;
if (final_dirs_result) {
    app_dirs = final_dirs_result.value();
}

After:

// in main.cpp
// Phase 1: Resolve only the config directory
auto config_dir_result = goggles::util::resolve_config_dir({});
auto config_dir = config_dir_result.value();

// Load config using just the config directory path
goggles::Config config = load_config_for_cli(cli_opts, config_dir);

// Phase 2: Resolve all directories once, with final overrides
auto final_overrides = goggles::util::overrides_from_config(config);
auto app_dirs_result = goggles::util::resolve_app_dirs(resolve_ctx, final_overrides);
const auto& app_dirs = app_dirs_result.value();
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a redundant and complex bootstrap sequence in main.cpp and proposes a more efficient, direct approach that aligns better with the newly introduced paths API design.

Medium
Possible issue
Fallback to template on copy failure

If copying the default configuration template fails, fall back to using the
template file directly for configuration instead of using hardcoded defaults.

src/app/main.cpp [223-237]

 if (!explicit_config && !exists) {
     const auto template_path =
         goggles::util::resource_path(bootstrap_dirs, "config/goggles.template.toml");
     if (std::filesystem::is_regular_file(template_path, ec) && !ec) {
         auto copy_result =
             copy_file_atomic(FileCopyPaths{.src = template_path, .dst = config_path});
         if (copy_result) {
             GOGGLES_LOG_INFO("Wrote default configuration: {}", config_path.string());
         } else {
             GOGGLES_LOG_WARN("Failed to write default configuration: {} ({})",
                              copy_result.error().message,
                              goggles::error_code_name(copy_result.error().code));
+            GOGGLES_LOG_INFO("Using template configuration: {}", template_path.string());
+            config_path = template_path;
         }
     }
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This suggestion improves the application's startup behavior by providing a more sensible fallback. Instead of silently using defaults when the user config can't be created, it uses the available template, which is more aligned with user expectations and the PR's goal of robust config handling.

Medium
General
Improve portability of fallback path

Replace the hardcoded /tmp fallback with a more portable solution using
preprocessor directives for different operating systems, and catch the specific
std::filesystem::filesystem_error instead of a generic catch(...).

src/util/paths.cpp [77-86]

 auto resolve_runtime_root() -> std::filesystem::path {
     if (auto env = get_env_path("XDG_RUNTIME_DIR")) {
         return *env;
     }
     try {
         return std::filesystem::temp_directory_path();
-    } catch (...) {
+    } catch (const std::filesystem::filesystem_error&) {
+#if defined(__linux__) || defined(__APPLE__)
         return "/tmp";
+#else
+        // A more portable fallback for other systems, e.g. Windows.
+        return std::filesystem::current_path();
+#endif
     }
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a hardcoded, non-portable path and an overly broad exception catch. The proposed change improves portability and makes the error handling more specific and robust.

Low
Improve portability of executable path resolution

Improve portability of get_exe_dir by replacing the Linux-specific readlink call
with a cross-platform implementation using preprocessor directives for systems
like macOS and providing a stub for others.

src/app/main.cpp [27-35]

+#if defined(__linux__)
+#include <unistd.h>
+#elif defined(__APPLE__)
+#include <mach-o/dyld.h>
+#endif
+
 static auto get_exe_dir() -> std::filesystem::path {
+#if defined(__linux__)
     std::array<char, 4096> exe_path{};
     const ssize_t len = readlink("/proc/self/exe", exe_path.data(), exe_path.size() - 1);
     if (len <= 0) {
         return {};
     }
     exe_path[static_cast<size_t>(len)] = '\0';
     return std::filesystem::path(exe_path.data()).parent_path();
+#elif defined(__APPLE__)
+    std::array<char, 4096> exe_path{};
+    uint32_t size = exe_path.size();
+    if (_NSGetExecutablePath(exe_path.data(), &size) != 0) {
+        return {};
+    }
+    return std::filesystem::path(exe_path.data()).parent_path();
+#else
+    // Other platforms (e.g., Windows) not supported yet.
+    return {};
+#endif
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a Linux-specific implementation and proposes a portable solution using preprocessor directives, which aligns with good practice for cross-platform development and improves the robustness of the new path resolution logic.

Low
Warn if preset directory missing

After determining the preset directory path, add a check to ensure it exists and
log a warning if it is not found.

src/app/ui_controller.cpp [104-109]

-std::filesystem::path preset_dir = util::data_path(app_dirs, "shaders/retroarch");
+std::filesystem::path data_preset = util::data_path(app_dirs, "shaders/retroarch");
 std::error_code ec;
+std::filesystem::path preset_dir;
+if (!std::filesystem::exists(data_preset, ec) || ec) {
+    preset_dir = util::resource_path(app_dirs, "shaders/retroarch");
+} else {
+    preset_dir = data_preset;
+}
 if (!std::filesystem::exists(preset_dir, ec) || ec) {
-    preset_dir = util::resource_path(app_dirs, "shaders/retroarch");
+    GOGGLES_LOG_WARN("Preset catalog directory not found: {}", preset_dir.string());
 }
 GOGGLES_LOG_INFO("Preset catalog directory: {}", preset_dir.string());
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion improves robustness by adding a check and a warning if the final resolved preset directory is not found. This provides better diagnostics for a potential misconfiguration or deployment issue.

Low
Avoid unnecessary string allocation

Explicitly create a std::string from the string_view before calling getenv to
ensure null-termination, instead of relying on a temporary.

src/util/paths.cpp [17-23]

 auto get_env(std::string_view key) -> std::optional<std::string> {
-    const char* value = std::getenv(std::string(key).c_str());
+    // std::string_view does not guarantee null-termination.
+    // Create a null-terminated string on the stack to be safe.
+    std::string key_str(key);
+    const char* value = std::getenv(key_str.c_str());
     if (value == nullptr || value[0] == '\0') {
         return std::nullopt;
     }
     return std::string(value);
 }
  • Apply / Chat
Suggestion importance[1-10]: 1

__

Why: The suggestion correctly identifies the need for a null-terminated string for getenv, but the existing code already handles this by creating a temporary std::string. The proposed change is functionally identical and offers only a marginal readability improvement.

Low
  • More

@K1ngst0m K1ngst0m merged commit b85f5cd into main Jan 15, 2026
4 checks passed
@K1ngst0m K1ngst0m deleted the dev/filesystem branch January 15, 2026 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant