refactor: improved type safety between js and cpp#63
Merged
Seele-Official merged 5 commits intoclice-io:mainfrom Mar 22, 2026
Merged
refactor: improved type safety between js and cpp#63Seele-Official merged 5 commits intoclice-io:mainfrom
Seele-Official merged 5 commits intoclice-io:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR converts Action and ExecutionEvent from optional-field structs to discriminated/tagged unions, introduces CommandCaptureResult as a success/error tagged result for command capture, implements a TaggedUnion/Bridge JS↔C++ conversion system, adds enum dispatch utilities, and updates call sites and tests to the new shapes. Changes
Sequence Diagram(s)sequenceDiagram
participant Capture as Command Capture (native)
participant Cpp as C++ core
participant JS as JS Bridge
participant Service as Service Callback
Capture->>Cpp: capture result (std::expected<CommandData,CatterErr>)
Cpp->>JS: convert to JS object { success: bool, data?, error? }
JS->>Service: invoke registered onCommand(id, CommandCaptureResult)
Service-->>JS: returns Action (tagged union)
JS->>Cpp: convert Action back to C++ TaggedUnion
Cpp->>Cpp: handle Action (dispatch by tag)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Seele-Official
changed the title
Contributor
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/common/util/enum.h (1)
15-20: Renamein_palce_enumbefore it leaks further.The misspelling is already propagating into call sites like
src/catter/core/ipc.cc, and it will be much more expensive to unwind once this helper is reused elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/util/enum.h` around lines 15 - 20, The helper struct name is misspelled as in_palce_enum; rename it to in_place_enum (and adjust any type usages and instantiations) so the correct identifier is used consistently; update the struct declaration (template <auto E> requires ... struct in_place_enum { using type = ...; constexpr static type value = E; };) and replace all call sites that reference in_palce_enum (e.g., places that instantiate or refer to its ::type or ::value) to the new in_place_enum symbol to avoid breaking references and ensure compilation.tests/unit/catter/core/js.cc (1)
73-75: Usethrow;after logging.Line 75 creates a new exception object instead of rethrowing the active one.
throw;preserves the original failure and avoids the extra copy.Suggested fix
- } catch(qjs::Exception& ex) { + } catch(const qjs::Exception& ex) { output::redLn("{}", ex.what()); - throw ex; + throw; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/catter/core/js.cc` around lines 73 - 75, In the catch block catching qjs::Exception& ex in tests/unit/catter/core/js.cc (the catch(qjs::Exception& ex) handler that logs ex.what()), don't rethrow by value; replace the current rethrow ("throw ex;") with a plain rethrow ("throw;") so the active exception is preserved and no copy is created.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/catter/core/capi/type.h`:
- Around line 78-86: The visit() overloads return by value due to auto and
thereby decay references from std::visit; change both visit(V&&) const and
non-const to use decltype(auto) so reference qualifiers are preserved when
std::visit yields a reference. Similarly, update get<T>() and get<const T>()
style accessors to return explicit reference types (e.g., T& / const T&) instead
of auto so callers receive the active variant by reference, and change
get_if<T>() to preserve pointer types (T* / const T*) rather than decayed
values; locate these in the type class around the visit(), get(), and get_if()
method definitions and update their return type declarations accordingly.
- Around line 243-250: The std::visit call in the static method to_js(JSContext*
ctx, const Union& union_value) is using the TaggedUnion instance directly, which
is not portable across C++20; change the visit target to the underlying
std::variant by calling union_value.variant() (i.e., invoke std::visit(...,
union_value.variant())) so the visit operates on the variant accessor provided
by Union/TaggedUnion and keeps consistency with other methods that use
variant().
In `@src/common/util/enum.h`:
- Around line 24-57: The dispatch(E e, F&& f) implementation incorrectly relies
on binary search over eventide::refl::reflection<E>::member_values (symbol:
Enum::member_values) which assumes ordering; replace the binary-search logic
with a simple linear scan over map (or Enum::member_values) to find the matching
enum by comparing underlying values and invoke the stored callback when found,
returning its result and throwing only if no match is found; additionally
correct the typographical error in the helper type/name from in_palce_enum to
in_place_enum everywhere it’s declared and used (references: function dispatch,
type in_palce_enum/in_place_enum and the lambda callbacks that construct
in_place_enum instances).
---
Nitpick comments:
In `@src/common/util/enum.h`:
- Around line 15-20: The helper struct name is misspelled as in_palce_enum;
rename it to in_place_enum (and adjust any type usages and instantiations) so
the correct identifier is used consistently; update the struct declaration
(template <auto E> requires ... struct in_place_enum { using type = ...;
constexpr static type value = E; };) and replace all call sites that reference
in_palce_enum (e.g., places that instantiate or refer to its ::type or ::value)
to the new in_place_enum symbol to avoid breaking references and ensure
compilation.
In `@tests/unit/catter/core/js.cc`:
- Around line 73-75: In the catch block catching qjs::Exception& ex in
tests/unit/catter/core/js.cc (the catch(qjs::Exception& ex) handler that logs
ex.what()), don't rethrow by value; replace the current rethrow ("throw ex;")
with a plain rethrow ("throw;") so the active exception is preserved and no copy
is created.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c9bdb0e-d05f-4e43-8914-25edd92eb1e6
📒 Files selected for processing (13)
api/catter-c/capi.d.tsapi/src/scripts/cdb.tsapi/src/service.tsapi/test/service.tssrc/catter/core/capi/type.hsrc/catter/core/ipc.ccsrc/catter/core/js.ccsrc/catter/core/js.hsrc/catter/core/qjs.hsrc/catter/main.ccsrc/common/util/enum.htests/unit/catter/core/js.cctests/unit/catter/core/type.cc
Contributor
|
❌ Failed to create PR with unit tests: AGENT_CHAT: Failed to open pull request |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/catter/main.cc (1)
59-67: Keep themodifypayload by reference.Line 59 uses plain
auto, soact.get<js::ActionType::modify>()is copied and the laterstd::movecalls only move from that copy. On this path that recreates a deep copy ofcwd/exe/argv/env.Suggested fix
- auto tag = act.get<js::ActionType::modify>(); + auto& tag = act.get<js::ActionType::modify>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/catter/main.cc` around lines 59 - 67, The code currently copies the modify payload by using plain "auto tag = act.get<js::ActionType::modify>()", causing std::move on tag.data.* to move from the copy and trigger deep copies; change the declaration to obtain a reference (e.g., "auto& tag = act.get<js::ActionType::modify>()" or "auto&& tag = act.get<...>()" as appropriate) so the subsequent std::move calls move from the original payload used to construct the data::action INJECT fields (cwd, exe, argv, env).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/catter/main.cc`:
- Around line 52-56: In the switch handling the JavaScript action in main.cc,
distinguish js::ActionType::drop from js::ActionType::skip: for
js::ActionType::drop return data::action with type set to data::action::DROP (no
cmd), and for js::ActionType::skip return data::action with type
data::action::INJECT and .cmd = cmd; update the switch arms around
js::ActionType::drop and js::ActionType::skip accordingly so drop is not mapped
to INJECT.
---
Nitpick comments:
In `@src/catter/main.cc`:
- Around line 59-67: The code currently copies the modify payload by using plain
"auto tag = act.get<js::ActionType::modify>()", causing std::move on tag.data.*
to move from the copy and trigger deep copies; change the declaration to obtain
a reference (e.g., "auto& tag = act.get<js::ActionType::modify>()" or "auto&&
tag = act.get<...>()" as appropriate) so the subsequent std::move calls move
from the original payload used to construct the data::action INJECT fields (cwd,
exe, argv, env).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2606539e-5fea-4c14-860b-39ed05ff9e39
📒 Files selected for processing (2)
src/catter/core/capi/type.hsrc/catter/main.cc
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/catter/core/capi/type.h (2)
216-225: Add constraint to avoid ambiguity with enum vector specialization.The general
Bridge<std::vector<T>>specialization (lines 216-225) lacks a constraint, which could cause ambiguity with the enum vector specialization at lines 203-214. While C++ partial specialization ordering typically resolves this correctly, adding an explicit!std::is_enum_v<T>constraint would make the intent clearer and prevent potential compilation issues.Suggested fix
template <typename T> + requires (!std::is_enum_v<T>) struct Bridge<std::vector<T>> { static std::vector<T> from_js(const qjs::Value& value) { return value.as<qjs::Array<T>>().template as<std::vector<T>>(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/catter/core/capi/type.h` around lines 216 - 225, The general Bridge<std::vector<T>> specialization can conflict with the enum-specific vector specialization; update the template for Bridge<std::vector<T>> (the struct named Bridge) to constrain T so it excludes enum types (e.g., require !std::is_enum_v<T>) using SFINAE or a requires clause; this ensures the general vector Bridge doesn't participate when T is an enum and removes ambiguity with the enum vector specialization.
3-3: Remove unnecessary<fcntl.h>include.Same issue as in
enum.h- this header provides POSIX file control operations which are not used here.Suggested fix
`#include` <cstdint> -#include <fcntl.h> `#include` <format>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/catter/core/capi/type.h` at line 3, Remove the unnecessary POSIX header include from type.h: delete the line that includes <fcntl.h> (same cleanup done in enum.h) since no fcntl functions or symbols are used in this header; ensure no other code in type.h references any fcntl-related macros before committing.src/common/util/enum.h (1)
4-4: Remove unnecessary<fcntl.h>include.This header provides POSIX file control operations (
open,fcntl, etc.) which are not used in this file. The include appears to be accidental.Suggested fix
`#include` <array> `#include` <cstddef> -#include <fcntl.h> `#include` <functional>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/util/enum.h` at line 4, Remove the unnecessary POSIX header by deleting the stray `#include` <fcntl.h> from src/common/util/enum.h; leave the remaining includes and declarations untouched so you don't alter symbols or dependencies, and run a quick build to ensure no other code relied on that accidental include.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/catter/core/capi/type.h`:
- Around line 216-225: The general Bridge<std::vector<T>> specialization can
conflict with the enum-specific vector specialization; update the template for
Bridge<std::vector<T>> (the struct named Bridge) to constrain T so it excludes
enum types (e.g., require !std::is_enum_v<T>) using SFINAE or a requires clause;
this ensures the general vector Bridge doesn't participate when T is an enum and
removes ambiguity with the enum vector specialization.
- Line 3: Remove the unnecessary POSIX header include from type.h: delete the
line that includes <fcntl.h> (same cleanup done in enum.h) since no fcntl
functions or symbols are used in this header; ensure no other code in type.h
references any fcntl-related macros before committing.
In `@src/common/util/enum.h`:
- Line 4: Remove the unnecessary POSIX header by deleting the stray `#include`
<fcntl.h> from src/common/util/enum.h; leave the remaining includes and
declarations untouched so you don't alter symbols or dependencies, and run a
quick build to ensure no other code relied on that accidental include.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d7b6a3e-5374-4585-8c99-a9f7c51f6bdf
📒 Files selected for processing (3)
src/catter/core/capi/type.hsrc/catter/core/ipc.ccsrc/common/util/enum.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Refactor