-
Notifications
You must be signed in to change notification settings - Fork 11
When following a pointer, adjust targetDoc to point to the root of the current doc #1805
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…e current doc, since our pointers may point outside of the portion where we are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/runner/src/traverse.ts">
<violation number="1" location="packages/runner/src/traverse.ts:524">
Potential runtime error: valueEntry.value may be null or non-object; guard with isObject before accessing ["value"].</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| // that's what our schema is relative to, so we'll grab the value part. | ||
| const targetDoc = { | ||
| address: { ...target, path: ["value"] }, | ||
| value: (valueEntry.value as Immutable<JSONObject>)["value"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential runtime error: valueEntry.value may be null or non-object; guard with isObject before accessing ["value"].
Prompt for AI agents
Address the following comment on packages/runner/src/traverse.ts at line 524:
<comment>Potential runtime error: valueEntry.value may be null or non-object; guard with isObject before accessing ["value"].</comment>
<file context>
@@ -492,43 +488,47 @@ function followPointer<S extends BaseMemoryAddress>(
+ // that's what our schema is relative to, so we'll grab the value part.
+ const targetDoc = {
+ address: { ...target, path: ["value"] },
+ value: (valueEntry.value as Immutable<JSONObject>)["value"],
+ };
</file context>
| value: (valueEntry.value as Immutable<JSONObject>)["value"], | |
| value: isObject(valueEntry.value) ? (valueEntry.value as Immutable<JSONObject>)["value"] : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps an argument for a better return type for manager.load, where we indicate that if the valueEntry returned has a defined value, that it will be an object with a value property.
However, I'm not going to address that in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion—refining manager.load’s return type to encode the value shape (e.g., via a discriminated union or type guard) would help avoid unsafe property access. Understood that it won’t be changed in this PR; I’ll keep this in mind for future reviews.
When following a pointer, adjust targetDoc to point to the root of the current doc, since our pointers may point outside of the portion where we are.
Summary by cubic
Fixes pointer traversal to start from the root of the target document so paths outside the current subdocument resolve correctly. Addresses CT-922 (traverse mismatched paths across docs).