refactor(util): consume goggles_core for error and profiling types by K1ngst0m · Pull Request #138 · goggles-dev/Goggles

K1ngst0m · 2026-03-25T04:51:29Z

Delete duplicated error.hpp and profiling.hpp from src/util/
Link goggles_core through goggles_util and visual test targets
Remove fragile GOGGLES_ERROR_TYPES_DEFINED ODR guards
Rewrite all include paths in src/ and tests/ to <goggles/error.hpp>
Update boundary contract test to reflect relocated canonical headers

Summary by CodeRabbit

Chores
- Consolidated error handling and profiling infrastructure across the codebase, updating module references and dependencies throughout source files.
- Updated submodule pointer to the latest version.
- Modified build configuration to reflect updated internal dependencies and module structure.
- Updated test files to align with new internal module organization.

- Delete duplicated error.hpp and profiling.hpp from src/util/ - Link goggles_core through goggles_util and visual test targets - Remove fragile GOGGLES_ERROR_TYPES_DEFINED ODR guards - Rewrite all include paths in src/ and tests/ to <goggles/error.hpp> - Update boundary contract test to reflect relocated canonical headers

coderabbitai · 2026-03-25T04:51:44Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2aad68a1-bcac-4819-ad99-9181ca2a35f6

📥 Commits

Reviewing files that changed from the base of the PR and between 34cf3b3 and a919e29.

📒 Files selected for processing (40)

filter-chain
src/app/application.cpp
src/app/application.hpp
src/app/cli.cpp
src/app/cli.hpp
src/app/main.cpp
src/compositor/compositor_core.cpp
src/compositor/compositor_cursor.cpp
src/compositor/compositor_focus.cpp
src/compositor/compositor_input.cpp
src/compositor/compositor_layer_shell.cpp
src/compositor/compositor_present.cpp
src/compositor/compositor_server.cpp
src/compositor/compositor_server.hpp
src/compositor/compositor_xdg.cpp
src/compositor/compositor_xwayland.cpp
src/render/backend/external_frame_importer.hpp
src/render/backend/filter_chain_controller.cpp
src/render/backend/render_output.hpp
src/render/backend/vulkan_backend.cpp
src/render/backend/vulkan_debug.cpp
src/render/backend/vulkan_debug.hpp
src/render/backend/vulkan_error.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/error.hpp
src/util/job_system.cpp
src/util/logging.cpp
src/util/logging.hpp
src/util/paths.cpp
src/util/paths.hpp
src/util/profiling.hpp
src/util/serializer.hpp
tests/render/test_filter_boundary_contracts.cpp
tests/util/test_error.cpp
tests/visual/CMakeLists.txt
tests/visual/image_compare.hpp

💤 Files with no reviewable changes (2)

src/util/profiling.hpp
src/util/error.hpp

📝 Walkthrough

Walkthrough

This PR migrates the codebase's error and profiling infrastructure from internal util/ headers to external goggles/ headers. It updates 35+ files' include directives, removes two utility headers, adjusts CMake link dependencies, and updates test references to align with the new module structure.

Changes

Cohort / File(s)	Summary
Error Header Migration `src/app/application.hpp`, `src/app/cli.hpp`, `src/compositor/compositor_server.hpp`, `src/render/backend/external_frame_importer.hpp`, `src/render/backend/render_output.hpp`, `src/render/backend/vulkan_debug.hpp`, `src/render/backend/vulkan_error.hpp`, `src/ui/imgui_layer.hpp`, `src/util/config.hpp`, `src/util/logging.hpp`, `src/util/paths.hpp`, `src/util/serializer.hpp`, `tests/visual/image_compare.hpp`	Replaced `<util/error.hpp>` includes with `<goggles/error.hpp>` across headers that use `Result<>` types in their public APIs; no signature changes.
Profiling Header Migration `src/app/application.cpp`, `src/app/cli.cpp`, `src/app/main.cpp`, `src/compositor/compositor_.cpp` (7 files), `src/render/backend/filter_chain_controller.cpp`, `src/render/backend/vulkan_.cpp` (2 files), `src/ui/imgui_layer.cpp`, `src/util/config.cpp`, `src/util/job_system.cpp`, `src/util/logging.cpp`, `src/util/paths.cpp`	Replaced `<util/profiling.hpp>` includes with `<goggles/profiling.hpp>` in implementation files using `GOGGLES_PROFILE_*` macros; no functional logic changes.
Removed Utility Headers `src/util/error.hpp`, `src/util/profiling.hpp`	Deleted error handling definitions (`ErrorCode`, `Error` struct, `Result<>` aliases, `GOGGLES_TRY`/`GOGGLES_MUST` macros) and profiling macro definitions (`GOGGLES_PROFILE_FRAME`, `GOGGLES_PROFILE_FUNCTION`, etc.).
CMake & Build Configuration `src/util/CMakeLists.txt`, `tests/visual/CMakeLists.txt`	Updated target link dependencies: replaced `nonstd::expected-lite` with `goggles_core` for both `goggles_util` and `image_compare` targets; added `goggles_core` to `goggles_util_logging_obj` private dependencies.
Test Updates & Contracts `tests/util/test_error.cpp`, `tests/render/test_filter_boundary_contracts.cpp`	Updated test includes from `util/error.hpp` to `goggles/error.hpp`; adjusted API boundary contract test to reference new canonical error header location and removed obsolete result header checks.
Submodule `filter-chain`	Updated submodule pointer from `28345abac...` to `0873141f7...` to pull in refactored filter-chain implementation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

PR #86: Added profiling instrumentation (GOGGLES_PROFILE_* macros) that directly depends on the profiling header structure being migrated in this PR.
PR #126: Updated filter-chain submodule with internal refactoring (ChainRuntime/ChainResources split), which aligns with the submodule pointer change in this PR.
PR #108: Introduced C++ filter-chain wrapper and header integration that now relies on the goggles/error.hpp and filter-chain headers being updated in this change.

Suggested labels

refactoring, build, header-migration

Poem

🐰 Headers hop from util/ to goggles/ with glee,
Error and profiling find their new home, you see!
No logic was changed, just paths rearranged,
Thirty-five files coordinated, beautifully staged,
The codebase now builds with headers arranged! 🎯

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'refactor(util): consume goggles_core for error and profiling types' accurately summarizes the main objective of the PR, which is to refactor the util module to use error and profiling types from goggles_core instead of duplicating them locally.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch dev/refactor-util

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.}

qodo-code-review · 2026-03-25T04:51:48Z

Review Summary by Qodo

Consume goggles_core for error and profiling types

✨ Enhancement

Walkthroughs

Description

• Eliminate duplicate error and profiling headers from src/util/
• Replace all local includes with goggles_core public headers
• Update CMake to link goggles_core instead of nonstd::expected-lite
• Remove fragile ODR guards from error type definitions
• Update boundary contract test to reflect canonical header locations

Diagram

flowchart LR
  A["src/util/error.hpp<br/>src/util/profiling.hpp"] -->|"Delete duplicates"| B["goggles_core<br/>canonical headers"]
  C["All source files<br/>include util/error.hpp"] -->|"Migrate to"| D["include goggles/error.hpp"]
  E["All source files<br/>include util/profiling.hpp"] -->|"Migrate to"| F["include goggles/profiling.hpp"]
  G["CMakeLists.txt<br/>nonstd::expected-lite"] -->|"Replace with"| H["goggles_core dependency"]

File Changes

1. src/util/error.hpp Refactoring +0/-162

Delete duplicate error type definitions

src/util/error.hpp

2. src/util/profiling.hpp Refactoring +0/-40

Delete duplicate profiling macro definitions

src/util/profiling.hpp

3. src/app/application.cpp Refactoring +1/-1

Update includes to use goggles namespace headers

src/app/application.cpp

View more (37)

4. src/app/application.hpp Refactoring +1/-1

Replace util/error.hpp with goggles/error.hpp

src/app/application.hpp

5. src/app/cli.cpp Refactoring +1/-1

Replace util/profiling.hpp with goggles/profiling.hpp

src/app/cli.cpp

6. src/app/cli.hpp Refactoring +1/-2

Replace util/error.hpp with goggles/error.hpp

src/app/cli.hpp

7. src/app/main.cpp Refactoring +2/-2

Update includes to use goggles namespace headers

src/app/main.cpp

8. src/compositor/compositor_core.cpp Refactoring +1/-1

Replace util/profiling.hpp with goggles/profiling.hpp

src/compositor/compositor_core.cpp

9. src/compositor/compositor_cursor.cpp Refactoring +1/-1

Replace util/profiling.hpp with goggles/profiling.hpp

src/compositor/compositor_cursor.cpp

10. src/compositor/compositor_focus.cpp Refactoring +1/-1

Replace util/profiling.hpp with goggles/profiling.hpp

src/compositor/compositor_focus.cpp

11. src/compositor/compositor_input.cpp Refactoring +1/-1

Replace util/profiling.hpp with goggles/profiling.hpp

src/compositor/compositor_input.cpp

12. src/compositor/compositor_layer_shell.cpp Refactoring +1/-1

Replace util/profiling.hpp with goggles/profiling.hpp

src/compositor/compositor_layer_shell.cpp

13. src/compositor/compositor_present.cpp Refactoring +1/-1

Replace util/profiling.hpp with goggles/profiling.hpp

src/compositor/compositor_present.cpp

14. src/compositor/compositor_server.cpp Refactoring +1/-1

Replace util/profiling.hpp with goggles/profiling.hpp

src/compositor/compositor_server.cpp

15. src/compositor/compositor_server.hpp Refactoring +1/-1

Replace util/error.hpp with goggles/error.hpp

src/compositor/compositor_server.hpp

16. src/compositor/compositor_xdg.cpp Refactoring +1/-1

Replace util/profiling.hpp with goggles/profiling.hpp

src/compositor/compositor_xdg.cpp

17. src/compositor/compositor_xwayland.cpp Refactoring +1/-1

Replace util/profiling.hpp with goggles/profiling.hpp

src/compositor/compositor_xwayland.cpp

18. src/render/backend/external_frame_importer.hpp Refactoring +1/-1

Replace util/error.hpp with goggles/error.hpp

src/render/backend/external_frame_importer.hpp

19. src/render/backend/filter_chain_controller.cpp Refactoring +1/-1

Replace util/profiling.hpp with goggles/profiling.hpp

src/render/backend/filter_chain_controller.cpp

20. src/render/backend/render_output.hpp Refactoring +1/-1

Replace util/error.hpp with goggles/error.hpp

src/render/backend/render_output.hpp

21. src/render/backend/vulkan_backend.cpp Refactoring +1/-1

Replace util/profiling.hpp with goggles/profiling.hpp

src/render/backend/vulkan_backend.cpp

22. src/render/backend/vulkan_debug.cpp Refactoring +1/-1

Replace util/profiling.hpp with goggles/profiling.hpp

src/render/backend/vulkan_debug.cpp

23. src/render/backend/vulkan_debug.hpp Refactoring +1/-1

Replace util/error.hpp with goggles/error.hpp

src/render/backend/vulkan_debug.hpp

24. src/render/backend/vulkan_error.hpp Refactoring +1/-1

Replace util/error.hpp with goggles/error.hpp

src/render/backend/vulkan_error.hpp

25. src/ui/imgui_layer.cpp Refactoring +1/-1

Replace util/profiling.hpp with goggles/profiling.hpp

src/ui/imgui_layer.cpp

26. src/ui/imgui_layer.hpp Refactoring +1/-1

Replace util/error.hpp with goggles/error.hpp

src/ui/imgui_layer.hpp

27. src/util/config.cpp Refactoring +1/-2

Replace local profiling.hpp with goggles/profiling.hpp

src/util/config.cpp

28. src/util/config.hpp Refactoring +1/-1

Replace local error.hpp with goggles/error.hpp

src/util/config.hpp

29. src/util/job_system.cpp Refactoring +1/-2

Replace local profiling.hpp with goggles/profiling.hpp

src/util/job_system.cpp

30. src/util/logging.cpp Refactoring +1/-2

Replace local profiling.hpp with goggles/profiling.hpp

src/util/logging.cpp

31. src/util/logging.hpp Refactoring +1/-2

Replace local error.hpp with goggles/error.hpp

src/util/logging.hpp

32. src/util/paths.cpp Refactoring +1/-1

Replace local profiling.hpp with goggles/profiling.hpp

src/util/paths.cpp

33. src/util/paths.hpp Refactoring +1/-2

Replace local error.hpp with goggles/error.hpp

src/util/paths.hpp

34. src/util/serializer.hpp Refactoring +1/-2

Replace util/error.hpp with goggles/error.hpp

src/util/serializer.hpp

35. tests/render/test_filter_boundary_contracts.cpp 🧪 Tests +2/-7

Update canonical header paths to goggles_core locations

tests/render/test_filter_boundary_contracts.cpp

36. tests/util/test_error.cpp Refactoring +1/-2

Replace util/error.hpp with goggles/error.hpp

tests/util/test_error.cpp

37. tests/visual/image_compare.hpp Refactoring +1/-1

Replace util/error.hpp with goggles/error.hpp

tests/visual/image_compare.hpp

38. src/util/CMakeLists.txt ⚙️ Configuration changes +2/-1

Link goggles_core instead of nonstd::expected-lite

src/util/CMakeLists.txt

39. tests/visual/CMakeLists.txt ⚙️ Configuration changes +1/-1

Link goggles_core instead of nonstd::expected-lite

tests/visual/CMakeLists.txt

40. filter-chain Dependencies +1/-1

Update submodule to consume goggles_core

filter-chain

qodo-code-review · 2026-03-25T04:51:49Z

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 📐 Spec deviations (0)

1. Brittle submodule header path 🐞 Bug ⛯ Reliability

Description

The boundary contract test now reads the canonical error header from a hardcoded submodule-internal
path (filter-chain/common/include/...), so the test fails if the filter-chain checkout layout
differs from this exact tree.

Code

tests/render/test_filter_boundary_contracts.cpp[R367-368]

+    const auto canonical_error_hpp =
+        std::filesystem::path(GOGGLES_SOURCE_DIR) / "filter-chain/common/include/goggles/error.hpp";

Evidence

The test constructs canonical_error_hpp from GOGGLES_SOURCE_DIR and a fixed relative path and then
requires the file contents exist (has_value()). This is coupled to the filter-chain source tree
layout rather than the installed/public interface, making it sensitive to submodule layout changes
or alternative GOGGLES_FILTER_CHAIN_SOURCE_DIR checkouts.

tests/render/test_filter_boundary_contracts.cpp[359-383]
CMakeLists.txt[16-31]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`tests/render/test_filter_boundary_contracts.cpp` hardcodes the filter-chain canonical error header path to `filter-chain/common/include/goggles/error.hpp`. The test reads this file from the source tree and fails when the filter-chain checkout layout differs.

## Issue Context
The project supports configuring `GOGGLES_FILTER_CHAIN_SOURCE_DIR` to point at an arbitrary filter-chain checkout, and the submodule layout can evolve. Boundary tests should validate the *public* headers / contracts without depending on submodule-internal directory structure.

## Fix Focus Areas
- tests/render/test_filter_boundary_contracts.cpp[359-383]

## Suggested changes
- Prefer checking the installed/public header path (e.g., under `filter-chain/include/...`) if that is the stable contract.
- If the new canonical header truly lives under `common/include`, make the test compute the path from `GOGGLES_FILTER_CHAIN_SOURCE_DIR` (add a CMake definition for it) rather than assuming it is always `${GOGGLES_SOURCE_DIR}/filter-chain/...`.
- Alternatively, change the test to validate the *include directives* in the canonical headers by including them (compile-time) rather than reading from the filesystem at runtime.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. Expected-lite still required 🐞 Bug ⚙ Maintainability

Description

CMake still unconditionally requires expected-lite even though goggles_util no longer links to
nonstd::expected-lite, so configuration can fail in environments where goggles_core is intended to
replace expected-lite.

Code

src/util/CMakeLists.txt[R32-37]

# Link dependencies (provided by Pixi)
target_link_libraries(goggles_util PUBLIC
-    nonstd::expected-lite
+    goggles_core
    spdlog::spdlog
    toml11::toml11
    BS_thread_pool

Evidence

cmake/Dependencies.cmake still has find_package(expected-lite REQUIRED) while
src/util/CMakeLists.txt switched from linking nonstd::expected-lite to goggles_core. If
goggles_core is the new canonical provider of error/result types, keeping expected-lite as a
required dependency is inconsistent and can cause unnecessary configure-time failures.

cmake/Dependencies.cmake[26-34]
src/util/CMakeLists.txt[12-39]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The build still unconditionally requires `expected-lite` via `find_package(expected-lite REQUIRED)` even though the refactor moves error/result ownership to `goggles_core` and removes explicit linkage to `nonstd::expected-lite` from `goggles_util`.

## Issue Context
If `goggles_core` now provides the canonical `Result`/`Error` types, `expected-lite` should either:
- remain required because it is still used transitively/indirectly, or
- become optional/removed if it is no longer part of the dependency surface.

Right now the build requirement is unconditional, which may be stricter than necessary.

## Fix Focus Areas
- cmake/Dependencies.cmake[26-34]
- src/util/CMakeLists.txt[12-39]

## Suggested changes
- If expected-lite is no longer needed directly by this repository, drop `find_package(expected-lite REQUIRED)`.
- If it is still needed (e.g., goggles_core uses it), document that and/or link it explicitly where needed so the dependency is intentional and obvious.
- Consider guarding it behind a check/option (e.g., only require expected-lite when building against a core implementation that needs it).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

ⓘ The new review experience is currently in Beta. Learn more

qodo-code-review · 2026-03-25T05:13:56Z

tests/render/test_filter_boundary_contracts.cpp

+    const auto canonical_error_hpp =
+        std::filesystem::path(GOGGLES_SOURCE_DIR) / "filter-chain/common/include/goggles/error.hpp";


1. Brittle submodule header path 🐞 Bug ⛯ Reliability

The boundary contract test now reads the canonical error header from a hardcoded submodule-internal path (filter-chain/common/include/...), so the test fails if the filter-chain checkout layout differs from this exact tree.

Agent Prompt

## Issue description `tests/render/test_filter_boundary_contracts.cpp` hardcodes the filter-chain canonical error header path to `filter-chain/common/include/goggles/error.hpp`. The test reads this file from the source tree and fails when the filter-chain checkout layout differs. ## Issue Context The project supports configuring `GOGGLES_FILTER_CHAIN_SOURCE_DIR` to point at an arbitrary filter-chain checkout, and the submodule layout can evolve. Boundary tests should validate the *public* headers / contracts without depending on submodule-internal directory structure. ## Fix Focus Areas - tests/render/test_filter_boundary_contracts.cpp[359-383] ## Suggested changes - Prefer checking the installed/public header path (e.g., under `filter-chain/include/...`) if that is the stable contract. - If the new canonical header truly lives under `common/include`, make the test compute the path from `GOGGLES_FILTER_CHAIN_SOURCE_DIR` (add a CMake definition for it) rather than assuming it is always `${GOGGLES_SOURCE_DIR}/filter-chain/...`. - Alternatively, change the test to validate the *include directives* in the canonical headers by including them (compile-time) rather than reading from the filesystem at runtime.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

K1ngst0m merged commit 8d3389b into main Mar 25, 2026
5 checks passed

K1ngst0m deleted the dev/refactor-util branch March 25, 2026 05:06

qodo-code-review bot reviewed Mar 25, 2026

View reviewed changes

coderabbitai bot mentioned this pull request Mar 25, 2026

chore: remove dead code, redundant comments, and test bloat #139

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(util): consume goggles_core for error and profiling types#138

refactor(util): consume goggles_core for error and profiling types#138
K1ngst0m merged 1 commit intomainfrom
dev/refactor-util

K1ngst0m commented Mar 25, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Mar 25, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Poem

Uh oh!

qodo-code-review bot commented Mar 25, 2026

Uh oh!

qodo-code-review bot commented Mar 25, 2026 •

edited

Loading

Uh oh!

Uh oh!

qodo-code-review bot Mar 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		const auto canonical_error_hpp =
		std::filesystem::path(GOGGLES_SOURCE_DIR) / "filter-chain/common/include/goggles/error.hpp";

Conversation

K1ngst0m commented Mar 25, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Poem

Uh oh!

qodo-code-review bot commented Mar 25, 2026

Review Summary by Qodo

Walkthroughs

File Changes

Uh oh!

qodo-code-review bot commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

qodo-code-review bot Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

K1ngst0m commented Mar 25, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Mar 25, 2026 •

edited

Loading

qodo-code-review bot commented Mar 25, 2026 •

edited

Loading