feat: fix qjs bugs , use JSException to distinguish error between js and cpp#61
Merged
Seele-Official merged 10 commits intoclice-io:mainfrom Mar 18, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds new QuickJS error types ( Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Client (C++)
participant Func as qjs::Function
participant QJS as QuickJS Runtime
participant Err as JS Error/Object
participant JSEx as catter::qjs::JSException
participant Logger as Logger
Caller->>Func: call(...)
Func->>QJS: execute JS code
QJS-->>Err: throws / rejects (Error value)
QJS->>JSEx: JSException::dump(ctx) (build JSException/Error)
JSEx-->>Func: propagate exception
Func-->>Caller: throw JSException
Caller->>Logger: log error (formatted with backticks)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
📝 Coding Plan
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 |
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/catter/core/js.cc (1)
106-125:⚠️ Potential issue | 🟠 MajorPreserve non-
Errorpromise rejection reasons.Both callbacks force every rejection argument through
arg.as<qjs::Error>(). If the rejection reason is a string, number,null, or a plain object, that conversion throws inside the callback and the wrapper ends up reporting its own failure instead of the original rejection.Suggested direction
+ auto append_rejection = [&](const qjs::Value& arg) { + if(auto error = arg.to<qjs::Error>(); error.has_value()) { + error_strace += error->format() + "\n"; + return; + } + if(auto text = arg.to<std::string>(); text.has_value()) { + error_strace += *text + "\n"; + return; + } + error_strace += "<non-Error rejection>\n"; + }; + auto reject = CallBack::from(js_ctx, [&](qjs::Parameters args) { state = Rejected; for(auto& arg: args) { - error_strace += arg.as<qjs::Error>().format() + "\n"; + append_rejection(arg); } }); @@ auto catch_fn = CallBack::from(js_ctx, [&](qjs::Parameters args) { state = Rejected; try { for(auto& arg: args) { - error_strace += arg.as<qjs::Error>().format() + "\n"; + append_rejection(arg); } } catch(const std::exception& e) { error_strace += std::format("Exception: {}\n", e.what()); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/catter/core/js.cc` around lines 106 - 125, The reject and catch_fn callbacks currently call arg.as<qjs::Error>() unconditionally which throws for non-Error rejection reasons; change both lambdas (the CallBack::from handlers named reject and catch_fn) to detect/handle non-Error values instead of forcing as<qjs::Error>() — e.g. check if arg is an Error (or use a safe conversion API) and if so append its formatted stack, otherwise convert the value to a stable string (JSON.stringify or the JS ToString representation) and append that to error_strace; ensure you don't let the conversion throw so the original rejection reason is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/api.md`:
- Line 1: doc/api.md currently contains a placeholder "TODO" which removes the
published API contract; replace it with the actual API documentation for the
public error surface and any changed exports so plugin authors have an upgrade
contract (restore the previous API content or regenerate the API doc to include
current exported functions/classes/errors and their signatures, and add a short
"Breaking changes / migration" section or link to the changelog/migration
guide). Ensure the file documents all public exports and error types referenced
by the codebase so consumers can rely on it.
In `@src/catter/core/capi/type.h`:
- Around line 129-133: The current logic around reading optional fields lets a
Value::Null slip through to from_property_value and throw; in the function using
object.get_optional_property(std::string(property_name)) (the optional_value
local), update the guard to return std::nullopt not only when
optional_value.has_value() is false but also when the contained Value is null
(e.g., optional_value->is_null() or equivalent), so that fields like { parent:
null } correctly map to std::nullopt before calling
from_property_value<value_type>(*optional_value).
In `@src/catter/core/qjs.h`:
- Around line 470-472: JSException::dump currently assumes the pending exception
is an Error; change it to first fetch the exception with JS_GetException(ctx)
and check JS_IsError(ctx, ex); if it's an Error keep the existing Error(ctx, ex)
path, otherwise convert the thrown value to a string (e.g. via
JS_ToString/JS_ToCString or JSON stringify) and construct the diagnostic from
that string (or wrap it into a new Error object) so non-Error throws like throw
"boom" or throw 1 are handled; make sure to manage JS values/cstrings lifetime
with JS_DupValue/JS_FreeValue/JS_FreeCString as appropriate.
In `@tests/unit/catter/core/qjs.cc`:
- Line 1: Remove the unused include by deleting the line containing "#include
<print>" from the file; this header is not referenced elsewhere in this test
(qjs.cc) so simply remove that include to keep includes clean.
---
Outside diff comments:
In `@src/catter/core/js.cc`:
- Around line 106-125: The reject and catch_fn callbacks currently call
arg.as<qjs::Error>() unconditionally which throws for non-Error rejection
reasons; change both lambdas (the CallBack::from handlers named reject and
catch_fn) to detect/handle non-Error values instead of forcing as<qjs::Error>()
— e.g. check if arg is an Error (or use a safe conversion API) and if so append
its formatted stack, otherwise convert the value to a stable string
(JSON.stringify or the JS ToString representation) and append that to
error_strace; ensure you don't let the conversion throw so the original
rejection reason is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 73adaeb2-b490-4ffb-a2f5-cc1a681ac04f
📒 Files selected for processing (8)
api/src/util/cmd.tsdoc/api.mdsrc/catter/core/apitool.hsrc/catter/core/capi/type.hsrc/catter/core/js.ccsrc/catter/core/qjs.htests/unit/catter/core/js.cctests/unit/catter/core/qjs.cc
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/js.cc`:
- Around line 106-111: The reject callback created via CallBack::from currently
calls arg.as<qjs::Error>() for each argument which can throw when the rejection
value is not an Error; wrap the per-argument conversion in a try-catch
(mirroring catch_fn) so you first attempt to convert to qjs::Error and on
exception fallback to using arg.to_string() (or a safe string conversion) before
appending to error_strace, keeping the state = Rejected behavior intact and
ensuring non-Error rejection values do not crash the code.
In `@tests/unit/catter/core/qjs.cc`:
- Line 508: The test calls a non-existent Error::format(); update the assertion
to use the actual method Error::to_string() instead — replace the use of
error.format() with error.to_string() in the EXPECT_TRUE check (the test around
EXPECT_TRUE(error.format().contains("Stack Trace:"))), ensuring the string
containment assertion still checks the returned std::string from
Error::to_string().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad592061-687b-4fe2-9246-a9975cdcf710
📒 Files selected for processing (3)
src/catter/core/js.ccsrc/catter/core/qjs.htests/unit/catter/core/qjs.cc
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/catter/core/js.cc (1)
106-111:⚠️ Potential issue | 🟠 MajorMissing try-catch in
rejectcallback for non-Error rejection values.The
rejectcallback at lines 106-111 callsarg.as<qjs::Error>().format()without exception handling, while thecatch_fncallback below (lines 117-126) correctly wraps the same pattern in a try-catch block.When a promise is rejected with a non-Error value (e.g.,
Promise.reject("string")orPromise.reject(42)),arg.as<qjs::Error>()will throwTypeExceptionbecauseobject_trans<Error>::as()checksJS_IsError()(seeqjs.h:1249-1254). This will crash instead of capturing the rejection reason.🐛 Proposed fix: Add try-catch like catch_fn
auto reject = CallBack::from(js_ctx, [&](qjs::Parameters args) { state = Rejected; - for(auto& arg: args) { - error_strace += arg.as<qjs::Error>().format() + "\n"; - } + try { + for(auto& arg: args) { + error_strace += arg.as<qjs::Error>().format() + "\n"; + } + } catch(const std::exception& e) { + error_strace += std::format("Exception: {}\n", e.what()); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/catter/core/js.cc` around lines 106 - 111, The reject callback created with CallBack::from (the lambda that sets state = Rejected and appends to error_strace by calling arg.as<qjs::Error>().format()) needs the same try-catch protection as the existing catch_fn: wrap the arg.as<qjs::Error>().format() call in a try-catch so non-Error rejection values (strings, numbers) don't throw TypeException; on exception, fall back to a safe string representation (e.g., convert arg to string or use a generic message) and append that to error_strace, mirroring the catch_fn error-handling pattern.src/catter/core/qjs.h (1)
470-472:⚠️ Potential issue | 🟠 Major
JSException::dump()assumes all JS exceptions areErrorobjects.This is a known issue from a previous review. When JavaScript throws a non-Error value (e.g.,
throw "boom",throw 1, orthrow {}), constructingError(ctx, JS_GetException(ctx))and callingformat()will fail becauseError::message(),Error::stack(), andError::name()call.as<std::string>()on properties that may not exist or may not be strings.🐛 Proposed fix with fallback for non-Error throws
inline JSException JSException::dump(JSContext* ctx) { - return JSException(Error(ctx, JS_GetException(ctx))); + JSValue ex = JS_GetException(ctx); + if(JS_IsError(ctx, ex)) { + return JSException(Error(ctx, std::move(ex))); + } + // Fallback for non-Error thrown values + const char* str = JS_ToCString(ctx, ex); + std::string msg = str ? str : "<non-Error JS exception>"; + if(str) JS_FreeCString(ctx, str); + JS_FreeValue(ctx, ex); + return JSException(Exception(std::move(msg))); }Note: This requires adding a constructor
JSException(Exception&&)orJSException(std::string&&)to handle the non-Error case.
🧹 Nitpick comments (2)
src/catter/core/qjs.h (2)
1309-1314:Functionarity check may throwJSExceptionunexpectedly.Line 1312 calls
obj.get_property("length").as<int64_t>()which can throwJSExceptionif the property access fails (e.g., due to a getter that throws). This exception would be unexpected when the intent is to throwTypeExceptionfor type mismatches.♻️ Consider using optional conversion
- if(obj.get_property("length").as<int64_t>() != sizeof...(Args)) { + auto len = obj.get_property("length").to<int64_t>(); + if(!len.has_value() || *len != sizeof...(Args)) { throw TypeException("Function has incorrect number of arguments"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/catter/core/qjs.h` around lines 1309 - 1314, The arity check currently calls obj.get_property("length").as<int64_t>() which can throw JSException; wrap the property access and conversion in a try/catch that catches JSException (or use a safe/optional retrieval API if available) and map any failure to the intended TypeException so callers always get TypeException for arity/type mismatches; update the code around obj.get_property("length") and the surrounding function (the function performing the Object-is-function check) to catch JSException and throw TypeException("Function has incorrect number of arguments") on error.
448-465: Consider defensive handling inErroraccessors.The
message(),stack(), andname()methods assume these properties exist and are strings. While standard JSErrorobjects have these properties, custom error objects or edge cases could causeTypeExceptionto be thrown from withinformat().This is partially mitigated if
JSException::dump()validates withJS_IsError()before constructing anError. However, ifErrorobjects are constructed directly elsewhere, consider adding defensive checks:♻️ Optional: Safer accessors with fallbacks
std::string message() const { - return this->get_property("message").as<std::string>(); + auto prop = this->get_optional_property("message"); + return prop ? prop->to<std::string>().value_or("<no message>") : "<no message>"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/catter/core/qjs.h` around lines 448 - 465, The Error accessor methods (message(), stack(), name()) assume the JS properties exist and are strings which can throw TypeException when used in format(); update these accessors in qjs.h (the Error class methods message, stack, name and the format() caller) to defensively check the property before converting: use get_property(...) return value checks (e.g., verify it's a string or has to_string() safely) or catch conversion errors and return a sensible fallback (empty string or "<unknown>") so format() never throws; ensure format() uses the safe accessors rather than performing raw conversions itself.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/catter/core/js.cc`:
- Around line 106-111: The reject callback created with CallBack::from (the
lambda that sets state = Rejected and appends to error_strace by calling
arg.as<qjs::Error>().format()) needs the same try-catch protection as the
existing catch_fn: wrap the arg.as<qjs::Error>().format() call in a try-catch so
non-Error rejection values (strings, numbers) don't throw TypeException; on
exception, fall back to a safe string representation (e.g., convert arg to
string or use a generic message) and append that to error_strace, mirroring the
catch_fn error-handling pattern.
---
Nitpick comments:
In `@src/catter/core/qjs.h`:
- Around line 1309-1314: The arity check currently calls
obj.get_property("length").as<int64_t>() which can throw JSException; wrap the
property access and conversion in a try/catch that catches JSException (or use a
safe/optional retrieval API if available) and map any failure to the intended
TypeException so callers always get TypeException for arity/type mismatches;
update the code around obj.get_property("length") and the surrounding function
(the function performing the Object-is-function check) to catch JSException and
throw TypeException("Function has incorrect number of arguments") on error.
- Around line 448-465: The Error accessor methods (message(), stack(), name())
assume the JS properties exist and are strings which can throw TypeException
when used in format(); update these accessors in qjs.h (the Error class methods
message, stack, name and the format() caller) to defensively check the property
before converting: use get_property(...) return value checks (e.g., verify it's
a string or has to_string() safely) or catch conversion errors and return a
sensible fallback (empty string or "<unknown>") so format() never throws; ensure
format() uses the safe accessors rather than performing raw conversions itself.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 138d9e15-a5a0-43b7-9a4c-941b0514d397
📒 Files selected for processing (2)
src/catter/core/js.ccsrc/catter/core/qjs.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
Improvements
Documentation
Tests