Optimize SurrealDB adapter #108
Merged
Conversation
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
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to cc54408 in 2 minutes and 30 seconds. Click for details.
- Reviewed
4456lines of code in33files - Skipped
0files when reviewing. - Skipped posting
9draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. tests/unit/queries/query.ts:2036
- Draft comment:
When testing for errors from asynchronous calls, avoid awaiting the result and then calling toThrow. Instead, use 'await expect(ctx.query(...)).rejects.toThrow(TypeError)'. This ensures the promise rejection is captured correctly. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. tests/unit/queries/query.ts:2198
- Draft comment:
Several tests are marked as TODO{TS} (e.g. batched queries with $as, deep nested queries with repeated paths, computed fields with missing dependencies). Consider using test.skip or grouping these tests so that unfinished scenarios do not cause false negatives. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. tests/unit/bench/bench.ts:12
- Draft comment:
The benchmarks repeatedly use deepSort and deepRemoveMetaData to normalize results. Consider extracting common result assertions into helper functions (or custom matchers) to reduce repetition and improve readability. - Reason this comment was not posted:
Comment was on unchanged code.
4. benches/v2.bench.ts:80
- Draft comment:
Typographical note: The comment says 'Pick in the middle of the b array + 2', but the code still uses '+ 1' (as seen in the previous bench). Please check if the comment should be updated to match the code or if the index calculation is incorrect. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a new file (benchmark file), so all code is new. The comment is pointing out an inconsistency between a code comment and the actual implementation. Line 72 says "+ 1" and uses+ 1. Line 80 says "+ 2" but uses+ 1. This could be either: (1) the comment is wrong and should say "+ 1", or (2) the code is wrong and should use+ 2. Since this is a benchmark file testing different scenarios, it would make sense to use different data points for each test. The automated tool is asking the author to verify which is correct. However, according to the rules, I should NOT keep comments that ask the author to confirm, verify, check, or ensure things. The comment says "Please check if the comment should be updated to match the code or if the index calculation is incorrect" which is explicitly asking the author to verify/check. While this does identify a real inconsistency, the rules explicitly state "Do NOT ask the PR author to confirm their intention, to explain, to double-check things, to ensure the behavior is intended" and this comment is doing exactly that - asking them to check/verify. The comment doesn't definitively state what the fix should be. The critique is valid. Even though there's a genuine inconsistency between the comment and code, the automated comment is phrased as a request for the author to verify/check rather than stating a definitive issue. According to the rules, this type of comment should be removed. This comment should be deleted because it asks the author to "check" and verify which is correct, rather than stating a definitive issue. It violates the rule against asking authors to confirm, verify, or double-check things.
5. src/stateMachine/query/surql2/buildSurql.ts:333
- Draft comment:
Typo in the JSDoc comment for the esc function: the comment reads "Escape identifier with for SurrealDB" which appears incomplete. Consider clarifying (e.g., "Escape identifier using angle brackets for SurrealDB") for better readability. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a documentation/comment improvement suggestion, not a code logic issue. The rules state "Do NOT comment unless there is clearly a code change required" and "Do NOT make comments that are obvious or unimportant." While the typo exists, fixing JSDoc comments is relatively minor and could be considered obvious/unimportant. However, the rules also say "Comments that suggest code quality refactors are good! But only if they are actionable and clear." This is actionable and clear. The key question is whether a JSDoc typo rises to the level of importance to keep the comment. Given that this is a new file being added, and the typo makes the documentation unclear about what escaping method is used, it could be valuable to fix. But it's borderline. This is just a documentation typo, not a functional issue. The code works fine regardless of the JSDoc comment. The rules emphasize not making obvious or unimportant comments, and a JSDoc typo might fall into that category. Additionally, the rule states to only comment when "there is clearly a code change required" - fixing documentation might not count as a required code change. While it's true this is just documentation, having clear and accurate documentation is part of code quality. The JSDoc is objectively incomplete/incorrect (missing text between "with" and "for"), and the suggestion is specific and actionable. However, given the emphasis in the rules on avoiding unimportant comments and only commenting when code changes are required, this likely doesn't meet the bar. This comment should be deleted. While the typo exists and the suggestion is valid, it's a minor documentation issue that doesn't affect functionality. The rules emphasize not making obvious or unimportant comments, and documentation typos typically fall into that category unless they cause significant confusion.
6. src/stateMachine/query/surql2/logical.ts:41
- Draft comment:
The comment starting with "This is the cardinality of" appears to be incomplete. Please complete or remove it for clarity. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to complete or remove an incomplete comment. This falls under asking the author to update the PR description or similar, which is against the rules. Therefore, this comment should be removed.
7. src/types/schema/enriched.draft.ts:2
- Draft comment:
Typo in the file header: "These types are design for SurrealDB query in mind." should likely read "These types are designed for SurrealDB queries in mind." - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, pointing out a typo in the file header. It doesn't provide a suggestion for code improvement or address any of the specific rules or guidelines provided.
8. tests/mocks/schema.ts:370
- Draft comment:
Typographical issue: There is a missing space in '2)replace only'. It would be clearer as '2) replace only' to maintain consistency with the other items listed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and focuses on a typographical issue, which does not impact the functionality or logic of the code. It does not align with the rules for useful comments, which should focus on code logic, functionality, or potential issues.
9. tests/unit/queries/query.ts:1485
- Draft comment:
Typo: There's an extra space in the commented out line (double space before 'email'). Consider reducing it to a single space for consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is about a minor formatting issue in a comment, which doesn't affect the functionality of the code. It doesn't provide a meaningful improvement to the code's logic or structure, and it doesn't align with the rules provided. Therefore, it should be removed.
Workflow ID: wflow_KIYPbEz2a8k0vrvu
You can customize 
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
40 issues found across 33 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/index.ts">
<violation number="1" location="src/index.ts:156">
P2: Rule violated: **Flag Security Vulnerabilities**
Logging full error objects during initialization can expose sensitive data (connection strings, credentials) in production logs. Consider sanitizing the error output or using a structured logger that filters sensitive information.</violation>
<violation number="2" location="src/index.ts:210">
P1: Rule violated: **Check System Design and Architectural Patterns**
The condition `Date.now() > 0` is always true and appears to be a debugging hack rather than a proper feature flag. This architectural anti-pattern:
- Forces the new code path without proper configuration controls
- Makes the branching logic meaningless (always takes new path when surrealDBClient exists)
- Bypasses existing `runQueryMachine` entirely without a proper toggle mechanism
Consider using a proper feature flag via configuration (e.g., `qConfig.useSurrealDbQueryMachine2`) or environment variable to control this behavior.</violation>
</file>
<file name="src/stateMachine/query/surql2/optimize.ts">
<violation number="1" location="src/stateMachine/query/surql2/optimize.ts:96">
P2: Rule violated: **Check System Design and Architectural Patterns**
DRY violation: Role field validation logic is duplicated in `convertRefFilterToRelationshipTraversal` (lines 85-94) and `convertNestedFilterToRelationshipTraversal` (lines 119-128). Extract this into a helper function like `validateRoleFieldForTraversal` to avoid maintenance issues.</violation>
<violation number="2" location="src/stateMachine/query/surql2/optimize.ts:160">
P2: Rule violated: **Check System Design and Architectural Patterns**
Dead code: `optimizeProjection` and `optimizeProjectionField` are defined but never called. The main entry point `optimizeLogicalQuery` passes through `query.projection` without optimization (line 14). Either wire up the projection optimization or remove these unused functions to avoid maintenance burden.</violation>
<violation number="3" location="src/stateMachine/query/surql2/optimize.ts:173">
P2: Wrong `thing` passed for nested reference optimization. The parent `thing` is used instead of resolving the nested field's target from the schema. This would cause incorrect filter optimization for nested references.</violation>
</file>
<file name="benches/v2.bench.ts">
<violation number="1" location="benches/v2.bench.ts:12">
P2: Rule violated: **Flag Security Vulnerabilities**
Hardcoded credentials detected. Even in benchmark/test code, database credentials should be loaded from environment variables to prevent accidental exposure in version control and to follow security best practices. Use `process.env` with dotenv to load these values.</violation>
<violation number="2" location="benches/v2.bench.ts:73">
P2: Comment says "+ 2" but code uses `+ 1`. This appears to be a copy-paste error from the previous bench test. Both tests will query the same element, which may not be the intended behavior.</violation>
<violation number="3" location="benches/v2.bench.ts:101">
P2: Rule violated: **Check System Design and Architectural Patterns**
DRY violation: The `createSurql`, `connect`, and `createSurqlBaseSet` functions are duplicated from `v2-2.bench.ts`. Extract these shared benchmark utilities to a common module (e.g., `benches/utils/surrealdb-helpers.ts`) to avoid maintenance burden and ensure consistency across benchmark files.</violation>
</file>
<file name="src/enrichSchema.draft.ts">
<violation number="1" location="src/enrichSchema.draft.ts:227">
P2: Bug: `oppositeRole` is an object, so this error message will display `[object Object]`. Use `lf.targetRole` to show the role name instead.</violation>
<violation number="2" location="src/enrichSchema.draft.ts:258">
P1: Rule violated: **Check System Design and Architectural Patterns**
This fallback logic is documented as incorrect in the TODO comment above it. The code knowingly implements behavior that will cause bugs in query transformations (as noted: queries using this will produce incorrect results). Either fix the implementation or throw an error when `targetingRelation` is not found, rather than falling back to `targetingRole` which produces semantically incorrect queries.</violation>
<violation number="3" location="src/enrichSchema.draft.ts:281">
P2: Rule violated: **Check System Design and Architectural Patterns**
Console.log statements violate proper architectural layering. Include this diagnostic information in the error message instead, or use a proper logging abstraction. This debug output will pollute stdout in production.</violation>
</file>
<file name="benches/insertData.v3.ts">
<violation number="1" location="benches/insertData.v3.ts:6">
P1: Rule violated: **Flag Security Vulnerabilities**
Hardcoded database credentials detected. Use environment variables instead (the project has `dotenv` as a dependency). Credentials in source code can be exposed through version control history.</violation>
<violation number="2" location="benches/insertData.v3.ts:24">
P2: Database connection is never closed. After the query completes, call `db.close()` to release the WebSocket connection and prevent resource leaks.</violation>
<violation number="3" location="benches/insertData.v3.ts:41">
P1: Rule violated: **Check System Design and Architectural Patterns**
This file duplicates code that already exists in `./generateData.ts`. The project has an established pattern where `insertData.v2.ts` imports shared types and the `generateData` function from that module. Instead of duplicating ~70 lines of code (interfaces `Base`, `A`, `B`, `generateData` function, and `uid` function), import them from the existing shared module to maintain consistency and reduce maintenance burden.</violation>
<violation number="4" location="benches/insertData.v3.ts:119">
P2: Rule violated: **Ensure all TypeScript code adheres to ECMAScript 2025 standards**
Use `nanoid` instead of custom `uid()` with `Math.random()`. The project already has `nanoid@^5.1.5` as a dependency, which is more secure (uses `crypto.getRandomValues`), more performant, and follows modern best practices for ID generation. `Math.random()` is not cryptographically secure and is considered a risky pattern for generating unique identifiers.</violation>
</file>
<file name="benches/rundb.v3.sh">
<violation number="1" location="benches/rundb.v3.sh:1">
P1: Add `set -euo pipefail` after the shebang to ensure the script exits on errors. This is a critical best practice for shell scripts to prevent silent failures.</violation>
<violation number="2" location="benches/rundb.v3.sh:5">
P1: Rule violated: **Flag Security Vulnerabilities**
Hardcoded credentials detected. The username and password (`borm_bench/borm_bench`) are hardcoded in the script. Even for benchmarking scripts, credentials should be sourced from environment variables (e.g., `${DB_USER:-default}`) to prevent accidental exposure and allow secure configuration.</violation>
<violation number="3" location="benches/rundb.v3.sh:49">
P3: Add a newline at the end of the file. POSIX requires text files to end with a newline character.</violation>
</file>
<file name="benches/insertData.v2.ts">
<violation number="1" location="benches/insertData.v2.ts:8">
P1: Rule violated: **Flag Security Vulnerabilities**
Hardcoded credentials detected. Even in benchmark files, passwords should not be committed to source control. Use environment variables with dotenv (already a project dependency) instead:
```typescript
const USERNAME = process.env.SURREAL_USERNAME ?? 'borm_bench';
const PASSWORD = process.env.SURREAL_PASSWORD ?? 'borm_bench';
```</violation>
<violation number="2" location="benches/insertData.v2.ts:22">
P2: Database connection is never closed after use. Consider calling `db.close()` after the query completes to properly release resources.</violation>
</file>
<file name="benches/rundb.v2.sh">
<violation number="1" location="benches/rundb.v2.sh:5">
P1: Rule violated: **Flag Security Vulnerabilities**
Hardcoded credentials detected. Even for benchmark/test scripts, credentials should be loaded from environment variables or a `.env` file (not committed to version control) rather than hardcoded in the script. This prevents accidental exposure and makes the script more secure.
Consider using:
```bash
USER=${SURREAL_USER:-borm_bench}
PASSWORD=${SURREAL_PASSWORD:-borm_bench}
```</violation>
<violation number="2" location="benches/rundb.v2.sh:22">
P2: Rule violated: **Flag Security Vulnerabilities**
The `--allow-all` flag grants all capabilities to SurrealDB, which is overly permissive. Even for benchmarking, consider specifying only the required capabilities explicitly to follow the principle of least privilege.</violation>
<violation number="3" location="benches/rundb.v2.sh:50">
P3: File is missing a newline at the end. POSIX requires text files to end with a newline character.</violation>
</file>
<file name="src/helpers.ts">
<violation number="1" location="src/helpers.ts:439">
P1: `Object.keys()` only returns string keys, never symbol keys. This filter will always return an empty array, so symbols will never be deleted. Use `Object.getOwnPropertySymbols(value)` instead.</violation>
</file>
<file name="benches/generateData.ts">
<violation number="1" location="benches/generateData.ts:27">
P2: Rule violated: **Ensure all TypeScript code adheres to ECMAScript 2025 standards**
String concatenation in loops is a less performant older pattern. Consider using `Array.from()` with `.join()` for more idiomatic modern JavaScript.</violation>
<violation number="2" location="benches/generateData.ts:60">
P1: Potential infinite loop: if `params.few.max` or `params.many.max` exceeds `params.records`, these while loops will never terminate because the Set cannot contain more unique IDs than exist in array `b`. Consider adding a guard condition or capping the length to `Math.min(fewLength, b.length)`.</violation>
<violation number="3" location="benches/generateData.ts:79">
P2: Rule violated: **Ensure all TypeScript code adheres to ECMAScript 2025 standards**
Use `nanoid` instead of custom `uid()` implementation. The project already has `nanoid@^5.1.5` as a dependency, which is a modern, cryptographically secure, and performant unique ID generator. The custom implementation using `Math.random()` is an older pattern that's less reliable.</violation>
</file>
<file name="src/stateMachine/query/surql2/buildLogical.ts">
<violation number="1" location="src/stateMachine/query/surql2/buildLogical.ts:72">
P2: Error message will display `[object Object]` because `field` is an object at this point. Use `field.$path` for a meaningful error message.</violation>
<violation number="2" location="src/stateMachine/query/surql2/buildLogical.ts:497">
P1: Incorrect operator mapping for `$contains` - it maps to `'NOT IN'` instead of a containment operator. The ternary `op === '$eq' ? 'IN' : 'NOT IN'` doesn't properly handle `$contains` and `$containsNot` cases.</violation>
</file>
<file name="benches/query.v3.ts">
<violation number="1" location="benches/query.v3.ts:8">
P2: Rule violated: **Flag Security Vulnerabilities**
Hardcoded credentials in source code. Even for benchmark files, use environment variables to avoid accidentally exposing secrets. Since the project includes `dotenv`, credentials should be loaded from environment variables (e.g., `process.env.USERNAME`).</violation>
<violation number="2" location="benches/query.v3.ts:22">
P2: Database connection is never closed after use. Call `db.close()` to properly release the connection resources before exiting. This prevents potential resource leaks, especially important if this benchmark pattern is reused elsewhere.</violation>
</file>
<file name="benches/schema.v2.surql">
<violation number="1" location="benches/schema.v2.surql:14">
P2: Type inconsistency: `array::first()` returns a single value, but fallback `|| []` returns an empty array. Consider using `|| NONE` instead to maintain consistent typing for a singular field.</violation>
<violation number="2" location="benches/schema.v2.surql:85">
P2: Logic bug: When `$var` is NONE/null, this returns `[null]` instead of an empty array `[]`. Consider adding a null check: `IF $var IS NONE THEN [] ELSE ...`</violation>
</file>
<file name="src/stateMachine/query/surql2/logical.ts">
<violation number="1" location="src/stateMachine/query/surql2/logical.ts:37">
P2: Misplaced JSDoc comment: This comment describes cardinality behavior but is positioned above the `filter` field instead of `cardinality`. Additionally, the actual `cardinality` field has an incomplete JSDoc comment that cuts off mid-sentence.</violation>
</file>
<file name="src/stateMachine/query/surql2/buildSurql.ts">
<violation number="1" location="src/stateMachine/query/surql2/buildSurql.ts:218">
P1: Bug: Double parameterization. `right[0]` is already a formatted string like `type::record($key)` from line 221, but it's being inserted as a parameter value again. This should directly use `right[0]` instead of re-parameterizing it.</violation>
<violation number="2" location="src/stateMachine/query/surql2/buildSurql.ts:286">
P1: Potential bug: `buildFilter` can return `undefined`, which would result in `NOT(undefined)` - invalid SQL. Add a null check before wrapping in NOT().</violation>
<violation number="3" location="src/stateMachine/query/surql2/buildSurql.ts:324">
P1: Rule violated: **Ensure all TypeScript code adheres to ECMAScript 2025 standards**
Replace custom `Math.random()`-based key generation with `nanoid`. The project already has `nanoid` as a dependency, which is a modern, secure, and performant solution for generating unique strings. `Math.random()` is not cryptographically secure and is considered a risky pattern for unique key generation.</violation>
<violation number="4" location="src/stateMachine/query/surql2/buildSurql.ts:337">
P1: Rule violated: **Flag Security Vulnerabilities**
Query injection vulnerability: The `esc` function does not escape the closing delimiter `⟩` within identifiers. An attacker-controlled identifier like `field⟩; DELETE * FROM users; --` would be wrapped as `⟨field⟩; DELETE * FROM users; --⟩`, breaking out of the escaping. Consider sanitizing `⟩` characters within identifiers (e.g., by escaping or rejecting them).</violation>
</file>
<file name="benches/v2-2.bench.ts">
<violation number="1" location="benches/v2-2.bench.ts:12">
P2: Rule violated: **Flag Security Vulnerabilities**
Hardcoded database credentials should be loaded from environment variables instead of being committed to source code. Even for benchmarks/tests, use `process.env` to avoid credential exposure in version control.</violation>
<violation number="2" location="benches/v2-2.bench.ts:175">
P2: Rule violated: **Flag Security Vulnerabilities**
Potential SurrealQL injection vulnerability: `data.string_1` is directly interpolated into the query without escaping special characters like quotes. Consider using parameterized queries or properly escaping string values.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if ((field.type !== 'role' && field.type !== 'link') || (filter.op !== 'IN' && filter.op !== 'CONTAINSANY')) { | ||
| return undefined; | ||
| } | ||
| if (field.type === 'role') { |
There was a problem hiding this comment.
P2: Rule violated: Check System Design and Architectural Patterns
DRY violation: Role field validation logic is duplicated in convertRefFilterToRelationshipTraversal (lines 85-94) and convertNestedFilterToRelationshipTraversal (lines 119-128). Extract this into a helper function like validateRoleFieldForTraversal to avoid maintenance issues.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/stateMachine/query/surql2/optimize.ts, line 96:
<comment>DRY violation: Role field validation logic is duplicated in `convertRefFilterToRelationshipTraversal` (lines 85-94) and `convertNestedFilterToRelationshipTraversal` (lines 119-128). Extract this into a helper function like `validateRoleFieldForTraversal` to avoid maintenance issues.</comment>
<file context>
@@ -0,0 +1,406 @@
+ if ((field.type !== 'role' && field.type !== 'link') || (filter.op !== 'IN' && filter.op !== 'CONTAINSANY')) {
+ return undefined;
+ }
+ if (field.type === 'role') {
+ // We can't do this optimization for role fields that are not played by a link field with target 'relation'.
+ // This relation is only used as intermediary relation.
</file context>
✅ Addressed in 75629e8
benches/v2.bench.ts
Outdated
| return db; | ||
| } | ||
|
|
||
| const createSurql = (data: { a: A[]; b: B[]; }): string => { |
There was a problem hiding this comment.
P2: Rule violated: Check System Design and Architectural Patterns
DRY violation: The createSurql, connect, and createSurqlBaseSet functions are duplicated from v2-2.bench.ts. Extract these shared benchmark utilities to a common module (e.g., benches/utils/surrealdb-helpers.ts) to avoid maintenance burden and ensure consistency across benchmark files.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At benches/v2.bench.ts, line 101:
<comment>DRY violation: The `createSurql`, `connect`, and `createSurqlBaseSet` functions are duplicated from `v2-2.bench.ts`. Extract these shared benchmark utilities to a common module (e.g., `benches/utils/surrealdb-helpers.ts`) to avoid maintenance burden and ensure consistency across benchmark files.</comment>
<file context>
@@ -0,0 +1,149 @@
+ return db;
+}
+
+const createSurql = (data: { a: A[]; b: B[]; }): string => {
+ const lines = ['BEGIN TRANSACTION;'];
+
</file context>
| const NAMESPACE = 'borm_bench'; | ||
| const DATABASE = 'borm_bench'; | ||
| const USERNAME = 'borm_bench'; | ||
| const PASSWORD = 'borm_bench'; |
There was a problem hiding this comment.
P2: Rule violated: Flag Security Vulnerabilities
Hardcoded credentials in source code. Even for benchmark files, use environment variables to avoid accidentally exposing secrets. Since the project includes dotenv, credentials should be loaded from environment variables (e.g., process.env.USERNAME).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At benches/query.v3.ts, line 8:
<comment>Hardcoded credentials in source code. Even for benchmark files, use environment variables to avoid accidentally exposing secrets. Since the project includes `dotenv`, credentials should be loaded from environment variables (e.g., `process.env.USERNAME`).</comment>
<file context>
@@ -0,0 +1,41 @@
+const NAMESPACE = 'borm_bench';
+const DATABASE = 'borm_bench';
+const USERNAME = 'borm_bench';
+const PASSWORD = 'borm_bench';
+
+const query = async () => {
</file context>
Collaborator
Author
There was a problem hiding this comment.
These are tests env.
benches/v2-2.bench.ts
Outdated
| const NAMESPACE = 'borm_bench'; | ||
| const DATABASE = 'borm_bench'; | ||
| const USERNAME = 'borm_bench'; | ||
| const PASSWORD = 'borm_bench'; |
There was a problem hiding this comment.
P2: Rule violated: Flag Security Vulnerabilities
Hardcoded database credentials should be loaded from environment variables instead of being committed to source code. Even for benchmarks/tests, use process.env to avoid credential exposure in version control.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At benches/v2-2.bench.ts, line 12:
<comment>Hardcoded database credentials should be loaded from environment variables instead of being committed to source code. Even for benchmarks/tests, use `process.env` to avoid credential exposure in version control.</comment>
<file context>
@@ -0,0 +1,176 @@
+const NAMESPACE = 'borm_bench';
+const DATABASE = 'borm_bench';
+const USERNAME = 'borm_bench';
+const PASSWORD = 'borm_bench';
+
+let client: BormClient;
</file context>
✅ Addressed in 736a0a1
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 978e4da in 1 minute and 9 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. benches/bench.ts:97
- Draft comment:
Switching 'first' to use variant.durations[0] ensures it reflects the first iteration’s timing rather than the minimum. Confirm this is the intended behavior. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking for confirmation of the intended behavior, which violates the rule against asking the PR author to confirm their intention. It does not provide a specific code suggestion or ask for a test to be written. Therefore, it should be removed.
Workflow ID: wflow_b7mNPgpcRGqy82hS
You can customize
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 081b5dc in 2 minutes and 41 seconds. Click for details.
- Reviewed
2990lines of code in30files - Skipped
0files when reviewing. - Skipped posting
8draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. tests/unit/queries/query.ts:2036
- Draft comment:
In the test for '[entity,includesAny,error]' (and similar tests for includesAll error), use await expect(...).rejects.toThrow(TypeError) instead of awaiting the result and then calling toThrow. This properly handles asynchronous rejections. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. tests/unit/queries/query.ts:1437
- Draft comment:
Tests labeled with 'TODO{TS}' or 'TODO{T}' should be marked as skipped using it.skip to avoid failing tests blocking the CI until the feature is implemented. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. tests/unit/queries/query.ts:27
- Draft comment:
Consider extracting large inline expected response objects into constants or helper functions to improve readability and maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. tests/unit/queries/query.ts:27
- Draft comment:
Instead of using 'expect(res).not.toBeInstanceOf(String)', consider using 'expect(typeof res).toBe("object")' for a clearer type assertion. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. tests/unit/queries/query.ts:104
- Draft comment:
Ensure helper functions like deepSort and deepRemoveMetaData are well-documented so that future maintainers understand their purpose and behavior. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. src/stateMachine/mutation/bql/enrichSteps/preHookValidations.ts:94
- Draft comment:
Typo detected in the inline comment. Consider changing 'Todo' to 'TODO' to follow standard conventions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. src/stateMachine/mutation/bql/flatter.ts:203
- Draft comment:
Typo: In the comment, "but lets do only replaces for now" should use an apostrophe to be "but let's do only replaces for now". - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and focuses on a minor typo in a comment, which doesn't affect the functionality or logic of the code. It doesn't provide any actionable feedback related to the code's functionality or structure.
8. src/stateMachine/query/surql2/buildSurql.ts:159
- Draft comment:
Typographical note: It appears that in the mapping block, the string "record::id($i)}" is missing a space before the closing brace. For consistency (and readability) it might be clearer to write it as "record::id($i) }". - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_HfTHAwbW59NfRoZn
You can customize
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 70c984a in 2 minutes and 44 seconds. Click for details.
- Reviewed
655lines of code in8files - Skipped
0files when reviewing. - Skipped posting
9draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/adapters/typeDB/schema/define.ts:47
- Draft comment:
Avoid using '@ts-expect-error' as a long-term solution. Fix the type error instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is about a change that was made in the diff (the addition of@ts-expect-error). The author already acknowledged this is a problem by adding "TODO: fix type error" in the code itself. The automated tool's comment is essentially repeating what the author already knows and documented. This falls under "obvious" comments - the author clearly knows this is a temporary solution that needs fixing. The comment doesn't provide any additional value beyond what the TODO already says. According to the rules, we should not make comments that are obvious or unimportant. However, the comment could be seen as reinforcing good practices and might prompt the author to fix it now rather than later. The rule about avoiding obvious comments might not apply if this is a code quality issue that should be addressed before merging. While reinforcing good practices has value, the author has already explicitly documented this as a TODO in the code itself. The comment provides no new information or actionable guidance beyond what's already there. It's redundant and obvious. This comment should be deleted because it's obvious and redundant - the author already added a TODO comment acknowledging the same issue. The automated tool is just repeating what the developer already knows.
2. src/index.ts:211
- Draft comment:
Remove redundant check 'Date.now() > 0' since it's always true. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. src/stateMachine/query/surql2/buildSurql.ts:16
- Draft comment:
Renaming the parameter from 'params' to 'mutParams' helps indicate mutation. However, consider minimizing in-place modifications or cloning the object to avoid unintended side-effects. - Reason this comment was not posted:
Comment was on unchanged code.
4. src/stateMachine/query/surql2/optimize.ts:426
- Draft comment:
The composite filter scoring calculation using reduce (a + a * b.score) seems off. Verify if the intended operation is multiplicative (e.g. acc * b.score) or additive, to ensure scores behave as expected. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. tests/multidb/mocks/schema.ts:93
- Draft comment:
The defaultDBConnector for the 'Project' entity is set with path 'Space'. Confirm if this is intentional or if it should be 'Project' to match the entity name. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. src/stateMachine/query/surql2/buildSurql.ts:307
- Draft comment:
The function doc comment still refers toparams, but the parameter has been renamed tomutParams. Please update the comment to reflect the new parameter name. - Reason this comment was not posted:
Comment was on unchanged code.
7. src/stateMachine/query/surql2/buildSurql.ts:328
- Draft comment:
Typo suggestion: There is an extra space between "Escape identifier." and "Only escapes". Please remove one space to improve readability. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and suggests a minor typographical change that does not impact the functionality or logic of the code. It does not align with the rules for making meaningful code suggestions or improvements.
8. src/stateMachine/query/surql2/optimize.ts:126
- Draft comment:
Typographical note: The comment says "for role fields that has no player with target 'relation'". Consider revising the subject-verb agreement (e.g., either "for a role field that has no player..." or "for role fields that have no player...") for clarity. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and focuses on a typographical note, which does not align with the rules provided. It does not suggest a code change or improvement, nor does it address any potential issues with the code's functionality or logic.
9. src/stateMachine/query/surql2/run.ts:16
- Draft comment:
Typographical error: The syntax on this line appears to be off. It currently reads "L16:) => {". It’s likely that it should be corrected to something like ") => {" (or include an appropriate colon and parentheses formatting) to match the intended arrow function syntax. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_hq8SSZReex0iZo4W
You can customize
There was a problem hiding this comment.
6 issues found across 8 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/stateMachine/query/surql2/optimize.ts">
<violation number="1" location="src/stateMachine/query/surql2/optimize.ts:226">
P2: `CONTAINSALL []` should be vacuously true (return `undefined`), not falsy. In set theory, every set contains all elements of the empty set. Only `IN` and `CONTAINSANY` with empty arrays should return falsy.</violation>
</file>
<file name="tests/multidb/mocks/schema.ts">
<violation number="1" location="tests/multidb/mocks/schema.ts:36">
P2: Invalid `targetRole` value. This link field connects to SpaceOwner relation which has roles `space` and `owner`. Since User plays 'owner', the opposite role should be `space`, not `ownedSpaces`.</violation>
<violation number="2" location="tests/multidb/mocks/schema.ts:44">
P2: Invalid `targetRole` value. This link field connects to SpaceMember relation which has roles `space` and `member`. Since User plays 'member', the opposite role should be `space`, not `spaces`.</violation>
<violation number="3" location="tests/multidb/mocks/schema.ts:52">
P2: Invalid `targetRole` value. This link field connects to ProjectExecutor relation which has roles `project` and `executor`. Since User plays 'executor', the opposite role should be `project`, not `projects`.</violation>
<violation number="4" location="tests/multidb/mocks/schema.ts:81">
P2: Invalid `targetRole` value. This link field connects to SpaceMember relation which has roles `space` and `member`. Since Space plays 'space', the opposite role should be `member`, not `members`.</violation>
<violation number="5" location="tests/multidb/mocks/schema.ts:118">
P2: Invalid `targetRole` value. This link field connects to ProjectExecutor relation which has roles `project` and `executor`. Since Project plays 'project', the opposite role should be `executor`, not `executors`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
tests/multidb/mocks/schema.ts
Outdated
| relation: 'ProjectExecutor', | ||
| plays: 'executor', | ||
| target: 'role', | ||
| targetRole: 'projects', |
There was a problem hiding this comment.
P2: Invalid targetRole value. This link field connects to ProjectExecutor relation which has roles project and executor. Since User plays 'executor', the opposite role should be project, not projects.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/multidb/mocks/schema.ts, line 52:
<comment>Invalid `targetRole` value. This link field connects to ProjectExecutor relation which has roles `project` and `executor`. Since User plays 'executor', the opposite role should be `project`, not `projects`.</comment>
<file context>
@@ -33,20 +33,23 @@ const typeDBSchema: BormSchema = {
relation: 'ProjectExecutor',
plays: 'executor',
target: 'role',
+ targetRole: 'projects',
},
],
</file context>
Suggested change
| targetRole: 'projects', | |
| targetRole: 'project', |
✅ Addressed in 9e63ea6
There was a problem hiding this comment.
7 issues found across 17 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="benches/v2.bench.ts">
<violation number="1" location="benches/v2.bench.ts:183">
P3: Comment typo: "full_one" should be "fut_one" to match the actual filter being tested.</violation>
</file>
<file name="src/stateMachine/query/tql/machine.ts">
<violation number="1" location="src/stateMachine/query/tql/machine.ts:127">
P0: State machine flow bug: `parse` state transitions directly to `'success'`, making the newly added `postHooks` and `clean` states unreachable. The transition should go to `'postHooks'` instead to ensure virtual fields are computed and response cleanup is performed.</violation>
</file>
<file name="package.json">
<violation number="1" location="package.json:76">
P2: `tinybench` is a benchmarking library used only for development/testing. It should be in `devDependencies` instead of `dependencies` to avoid unnecessary installation by package consumers.</violation>
</file>
<file name="tests/unit/bench/tinyBench.ts">
<violation number="1" location="tests/unit/bench/tinyBench.ts:676">
P1: Missing `return` statement - the `client.query()` result is not returned. This means all benchmark tasks receive `undefined` instead of actual query results, potentially making benchmarks less representative of real usage.</violation>
</file>
<file name="tests/benchTests.sh">
<violation number="1" location="tests/benchTests.sh:20">
P3: Typo in comment: 'argumentsppa' should be 'arguments'.</violation>
<violation number="2" location="tests/benchTests.sh:61">
P2: Unquoted `$(pwd)` can break if the path contains spaces. Quote the volume mount path to handle paths with special characters.</violation>
<violation number="3" location="tests/benchTests.sh:63">
P1: Missing spaces around `==` breaks the container readiness check. The condition is parsed as a single non-empty string (always true), so the `until` loop never executes. Also, prefer `$()` over backticks for command substitution.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
tests/unit/bench/tinyBench.ts
Outdated
| const { client, cleanup } = await init(); | ||
|
|
||
| const query: QueryFn = async (query, config) => { | ||
| client.query(query, { ...config, legacySurrealDBAdapter: LEGACY_SURREALDB_ADAPTER }); |
There was a problem hiding this comment.
P1: Missing return statement - the client.query() result is not returned. This means all benchmark tasks receive undefined instead of actual query results, potentially making benchmarks less representative of real usage.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/unit/bench/tinyBench.ts, line 676:
<comment>Missing `return` statement - the `client.query()` result is not returned. This means all benchmark tasks receive `undefined` instead of actual query results, potentially making benchmarks less representative of real usage.</comment>
<file context>
@@ -0,0 +1,709 @@
+ const { client, cleanup } = await init();
+
+ const query: QueryFn = async (query, config) => {
+ client.query(query, { ...config, legacySurrealDBAdapter: LEGACY_SURREALDB_ADAPTER });
+ };
+
</file context>
✅ Addressed in e4f9e28
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed ea74b6e in 8 minutes and 4 seconds. Click for details.
- Reviewed
833lines of code in3files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. tests/benchTests.sh:20
- Draft comment:
Typo in comment: 'argumentsppa' should be 'arguments'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% This is a straightforward typo fix. The comment is about a change (since this is a new file), it's actionable, and it's clearly correct. However, I need to check if this violates any rules. The rules say "Do NOT comment unless there is clearly a code change required" and "Do NOT make comments that are obvious or unimportant." A typo in a comment is technically not a code change - it's just a comment fix. Typos in comments could be considered "obvious or unimportant" since they don't affect functionality. However, fixing typos does improve code quality and readability. The question is whether this rises to the level of importance to warrant keeping the comment. This is just a typo in a comment, not in actual code. It doesn't affect functionality at all. The rules emphasize not making obvious or unimportant comments, and a typo in a comment could fall into that category. It's debatable whether this is important enough to keep. While it's true that this doesn't affect functionality, fixing typos does improve code quality and professionalism. The comment is clear, actionable, and correct. It's a minor fix but still a valid improvement. However, given the emphasis on only keeping comments that are clearly important and the rule about not making obvious or unimportant comments, this might be too minor to keep. This comment identifies a legitimate typo, but it's in a comment (not code) and doesn't affect functionality. Given the rules emphasizing not making obvious or unimportant comments, and that typos in comments are relatively minor issues, I should lean toward deleting this comment.
2. tests/benchTests.sh:20
- Draft comment:
Typo: 'argumentsppa' should be 'arguments' in the comment. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% This is a straightforward typo fix in a comment. The rules state "Do NOT make comments that are obvious or unimportant." A typo in a comment could be considered minor/unimportant since it doesn't affect functionality. However, the rules also say "Comments that suggest code quality refactors are good! But only if they are actionable and clear." This is actionable and clear. The key question is whether fixing a typo in a comment is "important" enough. Typos in comments can reduce code readability and professionalism, but they don't affect functionality. The rules emphasize not commenting unless there's "clearly a code change required" - but this is more of a documentation/comment quality issue rather than a functional code issue. While the typo is real and the fix is trivial, this might fall under "obvious or unimportant" comments that should be avoided. It's just a comment typo that doesn't affect code behavior, and the author could catch this themselves or it could be fixed in a later cleanup. However, the comment is actionable, clear, and improves code quality. It's a legitimate issue that was introduced in this PR. While minor, fixing typos maintains professionalism and readability. The rules don't explicitly forbid typo corrections, and this doesn't fall into any of the specific "do not" categories. This is a borderline case. The typo is real and the fix is trivial and actionable, but it's also minor and doesn't affect functionality. Given the emphasis on avoiding "obvious or unimportant" comments and only commenting when "clearly a code change required," I lean toward this being too minor to keep.
3. tests/unit/bench/tinyBench.ts:40
- Draft comment:
Typographical error: The key 'opt1[options, noMetadata' appears to be missing a closing bracket. It should likely be 'opt1[options, noMetadata]'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% This is a new file being added (all lines are additions in the diff). The comment is about a string literal that's used as an object key. While it's true that the bracket is unclosed, this is purely a naming/labeling issue, not a functional bug. The code will work fine either way since it's just a string. Looking at the rules, I need to consider: 1) Is this clearly a code change required? It's more of a style/consistency issue. 2) Is it obvious or unimportant? It's somewhat minor. 3) The rules say "Do NOT comment unless there is clearly a code change required" and "Do NOT make comments that are obvious or unimportant." This seems to fall into a gray area - it's a valid observation about consistency, but it's not a functional issue. However, looking more carefully, this appears to be a test naming convention where the brackets contain metadata about what's being tested. Having inconsistent bracket closure could make it harder to parse or search for these test names programmatically. This could be considered a legitimate code quality issue. This might actually be intentional - perhaps the author is using an unclosed bracket to indicate something specific, or it's part of a TODO pattern (as seen in line 45). I might be assuming this is an error when it could be deliberate. Also, this is purely cosmetic and doesn't affect functionality at all. While it could be intentional, the presence of many other similar keys with properly closed brackets (like 'opt3a[options, returnNull]') suggests this is more likely an oversight. Even if cosmetic, maintaining consistency in naming conventions is a reasonable code quality concern, especially in test suites where names are used for identification and reporting. This comment points out a legitimate consistency issue in the naming convention. While not a functional bug, it's a reasonable code quality suggestion that improves consistency. However, given the rules emphasize not making obvious or unimportant comments, and this is purely cosmetic, I'm leaning toward deleting it.
4. tests/unit/bench/tinyBench.ts:57
- Draft comment:
Typographical error: In the key 'opt3b[options, returnNull] - empty fields option in entity, dont return explicit', "dont" should be "don't". - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and focuses on a typographical error, which does not impact the functionality or logic of the code. It does not provide a suggestion for code improvement or address any potential issues with the code's behavior.
5. tests/unit/bench/tinyBench.ts:398
- Draft comment:
Typographical error: In the key 's1[self] Relation playing a a role defined by itself', there's a duplicate 'a'. It should read 'Relation playing a role defined by itself'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a comment about a typo in a string literal that serves as a test case name. According to the rules, I should not comment on things that are "obvious or unimportant." A typo in a test name is relatively minor - it doesn't affect functionality, just readability. The rules also state "Do NOT comment unless there is clearly a code change required." While fixing a typo is technically a code change, it's a very minor one. However, the rules also say comments about code quality are good if they are "actionable and clear." This is actionable (fix the typo) and clear. But is it important enough? Test names should be clear and professional, so fixing typos in them has some value. However, this feels like it falls into the "obvious or unimportant" category - it's a minor typo that doesn't affect the code's behavior. While the typo is real and the fix is straightforward, this might be considered too minor to warrant a review comment. The rules explicitly say not to make comments that are "obvious or unimportant," and a typo in a test case name could fall into that category since it doesn't affect functionality. Although minor, typos in test names reduce code quality and professionalism. However, given the explicit rule against obvious/unimportant comments and that this doesn't affect functionality, this comment should likely be deleted as it's too trivial. This comment should be deleted. While the typo is real, it's a minor issue in a test case name that doesn't affect functionality. The rules explicitly state not to make obvious or unimportant comments, and this falls into that category.
Workflow ID: wflow_7PQC96RtJNvMm7tc
You can customize
…zing nested reference
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 04fcb07 in 1 minute and 33 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. tests/benchTests.sh:20
- Draft comment:
Typo in comment fixed: 'argumentsppa' corrected to 'arguments'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states that a typo was fixed, which does not align with the rules for useful comments.
Workflow ID: wflow_o7Rh7vZHO7msGIAy
You can customize
There was a problem hiding this comment.
2 issues found across 22 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/stateMachine/query/surql2/optimize.ts">
<violation number="1" location="src/stateMachine/query/surql2/optimize.ts:145">
P1: Missing filter operation check after refactoring. The original code restricted this optimization to `filter.op === 'IN' || filter.op === 'CONTAINSANY'`, but after extracting `getRolePlayer`, this check was lost. Filters with `NOT IN`, `CONTAINSALL`, or `CONTAINSNONE` will now be incorrectly converted to relationship traversals, producing wrong query results.</violation>
</file>
<file name="src/stateMachine/query/surql2/buildSurql.ts">
<violation number="1" location="src/stateMachine/query/surql2/buildSurql.ts:329">
P2: Rule violated: **Flag Security Vulnerabilities**
**Inconsistent escaping check**: `needsEscaping` uses `escaped` for the first regex test but `identifier` for the second test. Both should use `escaped` for consistent behavior when identifiers contain `⟩`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 3c36632 in 2 minutes and 42 seconds. Click for details.
- Reviewed
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/stateMachine/query/surql2/buildSurql.ts:329
- Draft comment:
Using theescapedvariable for both regex tests improves consistency. However, note that even if the regex onescapeddiscovers unwanted characters, the function still returns the originalidentifier(wrapped in ⟨ ⟩) rather than the sanitizedescapedvalue. Ensure this behavior is intentional and that appropriate test coverage exists for identifiers containing characters like '⟩' which get replaced with a backslash. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_ab3p32qbInbupIYl
You can customize
- Remove pushIfDefined anti-pattern, use functional .map().filter() instead - Add type guards (isResultObject, isNullish, isEmptyArray) for better type safety - Improve null/undefined handling consistency in processResults
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 5f6cf3c in 1 minute and 45 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/stateMachine/query/surql2/buildLogical.ts:473
- Draft comment:
The explicit type annotation for the map callback parameter was removed. While TS inference may suffice, in complex pipelines an explicit type (e.g. NestedBQLFilter) can improve clarity and maintainability. Consider re-adding it if that enhances readability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_nC8ZgduCJCOEn6Lg
You can customize
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Important
Introduces an optimized SurrealDB query pipeline with a logical planner, optimizer, and benchmarking suite for improved performance.
runSurrealDbQueryMachine2insurql2/run.tsfor optimized query execution.buildLogicalQuery,optimizeLogicalQuery, andbuildSurqlfor query processing.processResultsinsurql2/processResults.tsfor result handling.benchTests.shandtestsBench.tsfor performance testing.BormSchemaand related types to support new query features.DRAFT_EnrichedBormSchemainenriched.draft.tsfor draft schema handling.deepRemoveMetaDatatosrc/helpers.ts.QueryConfiginconfig/base.tsto includelegacySurrealDBAdapterflag.This description was created by
for 5f6cf3c. You can customize this summary. It will automatically update as commits are pushed.
Summary by cubic
Introduces a new SurrealDB query pipeline with a logical planner, optimizer, and batched SurQL execution to reduce query cost and improve performance. Adds a benchmark suite and Docker scripts to measure v2 vs v3 behavior.
New Features
bench:surrealdb,bench:tests:surrealdb, andbench:tests:surrealdb:legacy; newlegacySurrealDBAdapterflag to toggle the adapter.deepRemoveMetaDatamoved tosrc/helpers; mutation parser updated.Migration
targetRole(e.g., user-space, user-session).indexesarray now supported on entities and relations.deepRemoveMetaDatafromsrc/helpers.Written for commit 5f6cf3c. Summary will update on new commits.