chore: update deno to v2.1.2 #923
Conversation
Yohe-Am
force-pushed
the
chore/deno-2
branch
from
6f46266 to
15d7cbe
Compare
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive set of updates across multiple files in the project, focusing on version upgrades, dependency management, and code refinements. Key changes include updating Rust and Deno versions to 1.82.0 and 2.1.2 respectively, modifying GitHub Actions workflows to use a new installation method for the Changes
Sequence DiagramsequenceDiagram
participant Developer
participant CI/CD
participant Build System
participant Runtime
participant Dependencies
Developer->>CI/CD: Push changes
CI/CD->>Build System: Install dependencies
Build System->>Dependencies: Fetch updated versions
Build System->>Runtime: Configure environment
Runtime->>Runtime: Initialize V8 flags
CI/CD->>Build System: Run tests and lint
Build System-->>Developer: Report build status
The sequence diagram illustrates the high-level workflow of the changes, showing how the updated dependencies and configurations are integrated into the build and runtime processes. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (49)
src/mt_deno/src/lib.rs (2)
246-252: **Performing type checks **Calling
check_specifiersfor type validation is an excellent step. Consider whether partial failures or concurrency might need explicit handling or more granular error reporting.
272-273: **Passingdesc_parserto test executor **Passing the
desc_parseris clear. As a minor suggestion, consider logging or handling parser configuration errors for enhanced debugging, if applicable.tests/schema_validation/type_comparison_test.ts (1)
10-10: **Use ofcatch (err: any)**Specifying
anyclarifies the shape of the caught exception. If feasible, consider a narrower type or a custom error interface to improve type safety and debugging clarity.src/typegate/src/services/auth/protocols/internal.ts (1)
52-52: Consider using a more specific type thananyin the catch block.While this approach works, using
unknownorError(when certain) can improve type safety. Consider narrowing or casting the error instead of relying on theanytype.src/typegate/src/services/auth/protocols/jwt.ts (1)
60-67: Avoid usinganyfor error handling.Using
unknownor a more specific type can help catch issues at compile time. It's often best to narrow or cast the error.tests/utils/s3.ts (2)
40-40: Consider using a more specific type instead ofany.
While catching errors asanyis quick, it may obscure potential type checks. Consider using a more specific error type or a type guard if feasible.
58-58: Replicate the approach from the previous catch block.
Same feedback applies here. If you can narrow down the type of raised exceptions, you could make your error handling more robust.tools/list-duplicates.ts (2)
41-41: Safe casting approach.
Castingtype.injectionasanyis understandable here. If you have a stable interface, consider using a stricter type.
55-55: Same reasoning aboutany.
To prevent losing type information, confirm thattype.injectionhas a known shape and, if possible, declare a dedicated interface for it.src/typegate/src/services/graphql_service.ts (1)
123-145: Ensure comprehensive logging and consistent error responses.All error branches now log and return a response with
err.message. Ensure that sensitive details are never leaked into logs. Otherwise, this is a clear, centralized approach to error handling.src/xtask/src/deno.rs (1)
54-79: Refine commented-out allowed commands.The large block of commented commands can make maintenance cumbersome. If you no longer need them, consider removing them permanently.
src/typegate/src/services/artifact_service.ts (1)
37-37: Consider usingunknowninstead ofany.While specifying a type for the caught error is an improvement over not specifying one at all, using
unknownin place ofanycan enhance type safety and reduce the risk of runtime type errors.src/typegate/engine/src/ext.rs (1)
142-146: Remove or clarify commented code.The commented-out permissions code might be confusing to future maintainers unless there is an intention to restore or replace it. Consider removing it entirely or adding an explanatory comment describing why it remains in the file and when it might be reintroduced.
Also applies to: 148-148
src/typegate/src/services/rest_service.ts (1)
Line range hint
92-129: Consistent renaming of error variables is clear and precise.Renaming
etoerrpromotes clarity in error handling. However, consider usingunknowninstead ofanyfor stronger type safety in TypeScript. Beyond this, the multiple checks forBaseError,ResolverError, andBadContextremain logically sound, ensuring appropriate HTTP response codes and JSON responses for each case.src/typegate/src/libs/jsonpath.ts (1)
203-203: Explicit error typing strengthens maintainability.Replacing an untyped catch variable with
catch (e: any)clarifies the nature of the error, though usingunknowncould provide even better type safety and help guard against unintentional usage of error properties.src/typegate/engine/src/lib.rs (1)
Line range hint
165-191: Consider removing commented-out defaultallow_importentries or converting them into configuration.Having large commented-out segments may cause confusion. If you anticipate using these default hosts, consider making them configurable or removing them altogether to keep your codebase clean and maintainable.
src/typegate/src/runtimes/prisma/hooks/run_migrations.ts (1)
110-110: Use a more precise error type to improve type-checking.Catching errors as
anysacrifices type safety. Consider usingunknownand then narrowing or wrapping the error, ensuring you capture exact information about failures.tests/e2e/cli/deploy_test.ts (2)
86-86: Consider narrowing the error type in the catch block.Catching exceptions as
anyis generally discouraged; usingunknownand then narrowing or type-guarding may improve error handling reliability.
142-142: Improve error typing.Again, consider using
unknownto avoid losing compile-time checks and enforce explicit type narrowing. This helps preserve important error details when handling edge cases.src/typegate/engine/bindings.ts (21)
59-59: Use more specific typing for error handling.Catching errors as
anyforfeits type safety. Preferunknownand then perform runtime checks or cast to a known error interface.
84-84: Enhance error type usage in catch.Using
anycan hide actual error structures. Switch tounknownand handle possible error cases explicitly.
108-108: Encourage explicit error typing.Relying on
anyleads to potential maintainability issues. If possible, replace withunknownand narrow the type.
127-127: Improve error typing for better clarity.Switch from
anytounknownor a known error type to leverage TypeScript’s type checks.
149-149: Refine error handling approach.Catching an
anytype can conceal important details; preferunknownplus a type guard or instance check.
171-171: Strengthen type safety in error handling.Change
anytounknownfor stricter compile-time checks, reducing potential runtime errors.
190-190: Favor narrower error types.Catching errors as
anymay hide critical type specifics, consider usingunknown.
211-211: Adopt more explicit error type.Maintain code clarity by dropping
anyin favor ofunknownor a specialized error type.
234-234: Increase error-type clarity.Use
unknownto encourage explicit checking or transformation of caught errors.
253-253: Use explicit error handling strategy.Replace
anywithunknown, forcing callers to handle error details properly.
275-275: Refine catch error typing.A narrower type than
anyhelps ensure that error properties are validated before use.
297-302: Leverage rigorous error typing.Both catch blocks here use
any. Reviewing them forunknownusage can improve clarity and safety.
339-339: Favor safer error typing.Handling errors as
unknownencourages robust validation and prevents accidental reliance on undefined properties.
362-362: Improve error stability with narrower types.Using
unknownensures that reading properties on the error is done in a controlled manner.
388-388: Enhance reliability with explicit error typing.Switching from
anytounknowncan help avoid unexpected runtime failures.
412-412: Strengthen error safety.
unknownis preferable overanyto guarantee proper type checks for errors.
431-431: Encourage a typed catch block.Use
unknownin place ofanyso that error checks become more deliberate.
454-454: Refactor catch clause for better type checks.Rely on a properly typed approach rather than
any, ensuring strong error validation.
473-473: Favor narrower error type to maintain clarity.By removing
any, you reduce accidental assumptions about error structure or fields.
496-496: Use typed errors for clarity & safety.Switch from
anytounknownto align with TypeScript best practices for error handling.
516-516: Adopt a narrower type in catch for consistency.Maintain consistent error handling throughout the codebase by ditching
anyforunknown.src/typegate/src/typegate/mod.ts (1)
215-215: Enhance unknown error messaging
Usingerr.toString()is acceptable for unknown errors, but to improve debuggability, consider adding more metadata (e.g., stack trace) to the error message or logs.tests/utils/test.ts (1)
357-357: Maintain consistent error-typing in test assertions
Typing the caught error asanyis fine for tests, but if possible, consider more specific error types to gain type safety and avoid masking real issues.src/typegate/src/engine/planner/policies.ts (1)
28-28: Clarify type usage for PolicyResolverOutput
Switching from(unknown & {})to(unknown & any)removes some of the safety introduced by stricter types. Consider whetheranyis required or if there is a more precise type that captures the intended usage..github/workflows/publish-website.yml (2)
32-32: Trailing spaces found
Remove the trailing spaces to avoid triggering linter warnings.- +🧰 Tools
🪛 yamllint (1.35.1)
[error] 32-3232: trailing spaces
(trailing-spaces)
34-34: Quote the GHJK_VERSION variable to prevent word splitting
ShellCheck warns about potential issues if the$GHJK_VERSIONvariable contains spaces. Even if unlikely, quoting is considered best practice.- - run: curl -fsSL https://raw.github.com/metatypedev/ghjk/$GHJK_VERSION/install.sh | bash + - run: curl -fsSL "https://raw.github.com/metatypedev/ghjk/$GHJK_VERSION/install.sh" | bash🧰 Tools
🪛 actionlint (1.7.4)
34-3434: shellcheck reported issue in this script: SC2086:info:1:52: Double quote to prevent globbing and word splitting
(shellcheck)
Cargo.toml (1)
62-62: Note about deprecated dependencyThe comment indicates that serde_yaml is deprecated. Consider tracking this as a future improvement.
.github/workflows/tests.yml (1)
173-173: Remove trailing whitespaceThere are trailing spaces on lines 173 and 284.
Also applies to: 284-284
🧰 Tools
🪛 yamllint (1.35.1)
[error] 173-173173: trailing spaces
(trailing-spaces)
.github/workflows/release.yml (1)
38-42: Consider using a composite action to reduce code duplication.The ghjk installation steps are repeated in multiple jobs. Consider creating a composite action to encapsulate these steps for better maintainability.
Example composite action:
# .github/actions/setup-ghjk/action.yml name: 'Setup GHJK' description: 'Installs GHJK' runs: using: "composite" steps: - run: curl -fsSL "https://raw.github.com/metatypedev/ghjk/${GHJK_VERSION}/install.sh" | bash shell: bash - run: echo "$HOME/.local/bin" >> "$GITHUB_PATH" shell: bash - run: echo "BASH_ENV=$HOME/.local/share/ghjk/env.sh" >> "$GITHUB_ENV" shell: bash - run: ghjk envs cook shell: bashUsage:
steps: - uses: ./.github/actions/setup-ghjkAlso applies to: 106-109, 186-190, 326-329
🧰 Tools
🪛 actionlint (1.7.4)
39-3939: shellcheck reported issue in this script: SC2086:info:1:52: Double quote to prevent globbing and word splitting
(shellcheck)
src/typegate/src/engine/planner/args.ts (1)
802-802: Consider using a more specific error type thanany.While adding a type annotation is good, using
anyreduces type safety. Consider usingunknownfor better type safety, or if possible, a more specific error type.- } catch (e: any) { + } catch (e: unknown) { const msg = e.message;If using
unknown, you'll need to add a type guard:} catch (e: unknown) { const msg = e instanceof Error ? e.message : String(e); throw new BadContext("Error while querying context: " + msg); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockdeno.lockis excluded by!**/*.lock
📒 Files selected for processing (51)
.ghjk/lock.json(13 hunks).github/workflows/publish-website.yml(1 hunks).github/workflows/release.yml(4 hunks).github/workflows/tests.yml(5 hunks)Cargo.toml(3 hunks)deno.jsonc(1 hunks)ghjk.ts(0 hunks)import_map.json(1 hunks)rust-toolchain.toml(1 hunks)src/meta-cli/src/cli/typegate.rs(1 hunks)src/metagen-client-rs/src/graphql.rs(2 hunks)src/mt_deno/src/lib.rs(6 hunks)src/substantial/src/backends/redis.rs(1 hunks)src/typegate/engine/bindings.ts(21 hunks)src/typegate/engine/src/ext.rs(1 hunks)src/typegate/engine/src/lib.rs(3 hunks)src/typegate/src/engine/planner/args.ts(1 hunks)src/typegate/src/engine/planner/policies.ts(1 hunks)src/typegate/src/libs/jsonpath.ts(1 hunks)src/typegate/src/runtimes/deno/deno.ts(1 hunks)src/typegate/src/runtimes/deno/deno_messenger.ts(0 hunks)src/typegate/src/runtimes/deno/hooks/mod.ts(2 hunks)src/typegate/src/runtimes/prisma/hooks/run_migrations.ts(1 hunks)src/typegate/src/runtimes/python/hooks/mod.ts(1 hunks)src/typegate/src/runtimes/typegate.ts(1 hunks)src/typegate/src/services/artifact_service.ts(2 hunks)src/typegate/src/services/auth/mod.ts(1 hunks)src/typegate/src/services/auth/protocols/internal.ts(1 hunks)src/typegate/src/services/auth/protocols/jwt.ts(1 hunks)src/typegate/src/services/auth/protocols/oauth2.ts(1 hunks)src/typegate/src/services/graphql_service.ts(2 hunks)src/typegate/src/services/rest_service.ts(3 hunks)src/typegate/src/typegate/mod.ts(1 hunks)src/typegate/standalone/src/main.rs(1 hunks)src/typegraph/deno/src/types.ts(0 hunks)src/xtask/src/deno.rs(1 hunks)tests/e2e/cli/deploy_test.ts(2 hunks)tests/e2e/typegraph/validator_test.ts(1 hunks)tests/planner/planner_test.ts(0 hunks)tests/schema_validation/type_comparison_test.ts(1 hunks)tests/utils/assert.ts(1 hunks)tests/utils/s3.ts(2 hunks)tests/utils/test.ts(1 hunks)tools/Dockerfile(2 hunks)tools/consts.ts(1 hunks)tools/deps.ts(1 hunks)tools/jsr/jsr-gen.ts(1 hunks)tools/list-duplicates.ts(3 hunks)tools/tasks/lint.ts(1 hunks)tools/tree-view.ts(2 hunks)tools/update.ts(2 hunks)
💤 Files with no reviewable changes (4)
- src/typegraph/deno/src/types.ts
- src/typegate/src/runtimes/deno/deno_messenger.ts
- ghjk.ts
- tests/planner/planner_test.ts
✅ Files skipped from review due to trivial changes (4)
- src/substantial/src/backends/redis.rs
- rust-toolchain.toml
- import_map.json
- tools/update.ts
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/tests.yml
93-9393: shellcheck reported issue in this script: SC2086:info:1:52: Double quote to prevent globbing and word splitting
(shellcheck)
145-145145: shellcheck reported issue in this script: SC2086:info:1:52: Double quote to prevent globbing and word splitting
(shellcheck)
175-175175: shellcheck reported issue in this script: SC2086:info:1:52: Double quote to prevent globbing and word splitting
(shellcheck)
176-176176: shellcheck reported issue in this script: SC2088:warning:1:7: Tilde does not expand in quotes. Use $HOME
(shellcheck)
176-176176: shellcheck reported issue in this script: SC2086:info:1:24: Double quote to prevent globbing and word splitting
(shellcheck)
177-177177: shellcheck reported issue in this script: SC2086:info:1:47: Double quote to prevent globbing and word splitting
(shellcheck)
288-288288: shellcheck reported issue in this script: SC2086:info:1:52: Double quote to prevent globbing and word splitting
(shellcheck)
289-289289: shellcheck reported issue in this script: SC2088:warning:1:7: Tilde does not expand in quotes. Use $HOME
(shellcheck)
289-289289: shellcheck reported issue in this script: SC2086:info:1:24: Double quote to prevent globbing and word splitting
(shellcheck)
290-290290: shellcheck reported issue in this script: SC2086:info:1:47: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/publish-website.yml
34-3434: shellcheck reported issue in this script: SC2086:info:1:52: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/release.yml
39-3939: shellcheck reported issue in this script: SC2086:info:1:52: Double quote to prevent globbing and word splitting
(shellcheck)
106-106106: shellcheck reported issue in this script: SC2086:info:1:52: Double quote to prevent globbing and word splitting
(shellcheck)
187-187187: shellcheck reported issue in this script: SC2086:info:1:52: Double quote to prevent globbing and word splitting
(shellcheck)
326-326326: shellcheck reported issue in this script: SC2086:info:1:52: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint (1.35.1)
.github/workflows/tests.yml
[error] 173-173173: trailing spaces
(trailing-spaces)
[error] 284-284284: trailing spaces
(trailing-spaces)
.github/workflows/publish-website.yml
[error] 32-3232: trailing spaces
(trailing-spaces)
.github/workflows/release.yml
[error] 37-3737: trailing spaces
(trailing-spaces)
[error] 185-185185: trailing spaces
(trailing-spaces)
[error] 324-324324: trailing spaces
(trailing-spaces)
🔇 Additional comments (44)
src/mt_deno/src/lib.rs (10)
44-44: Confirm usage ofPermissionFlags[approved, verify]Switching from
PermissionsOptionstoargs::PermissionFlagsaligns with the updated permission model. Check references throughout the codebase to ensure all calls are consistent and no unforeseen breakages occur.
67-67: **Consistent update toPermissionFlags**The parameter type change here maintains consistency with the rest of the permission handling across the file.
80-80: **Correct pass-through of updated permissions **Passing the
permissionsobject toargs::Flagsis coherent with the refactored permission system. No issues noted.
235-235: **Validate condition for empty test modules **The newly added
permit_no_filescheck ensures that tests with no specifiers are disallowed unless explicitly permitted. Verify that this logic matches intended behavior and doesn’t block legit scenarios where tests may be optional.
238-238: **doc_tests integration **Acquiring doc tests from file fetcher can streamline test coverage. No immediate issues spotted; looks good for ensuring doc-based tests are included.
239-239: **Consolidating target specifiers **Grouping specifiers for both type checking and test execution is a clear approach, centralizing these references for downstream operations.
240-240: **Iterating over doc tests **This loop is straightforward; doc tests are processed individually. No concerns with the iteration approach.
241-241: **Inserting doc tests into memory **Using
file_fetcher.insert_memory_files(doc_test)is clever. Verify that memory insertion is tested with various doc test scenarios to avoid potential caching issues.
261-267: **Isolated Permissions for tests **Ensuring each test obtains a fresh
Permissionsobject prevents unintended carry-over of granted or revoked permissions between tests. This design choice appears robust.
268-268: **Running test specifiers **Invoking
test_specifiersafter preparing isolated permissions is logical. Properly orchestrates final test steps.tests/utils/assert.ts (1)
23-23: **Catching error asany**This improves TypeScript clarity in the retry loop. It’s appropriate for a broad catch-all in test code, though stricter definitions can further enhance maintainability.
tests/e2e/typegraph/validator_test.ts (1)
30-30: **Catching error asany**In test cases, capturing any type of error is a practical approach for robust error debugging. Current usage is fine.
tools/tasks/lint.ts (1)
50-50: **Additionaldeno lintcommand **Executing
deno lintafterdeno checkensures thorough static analysis. This is a beneficial improvement, reinforcing code quality checks on each run.deno.jsonc (1)
41-41: Verify compatibility of"nodeModulesDir": "none".Ensure that Deno 2.1.2 fully supports
"none"here, as opposed to a boolean setting. Confirm there's no side effect on the project’s workflows, particularly around dependency resolution.tools/jsr/jsr-gen.ts (1)
5-5: Removal of unused import looks good.The
existsSyncimport appears unused, so removing it is a welcome cleanup step.src/typegate/standalone/src/main.rs (1)
44-49: Ensure robust error handling and environment validation.Calling
init_v8_flagsis beneficial for properly initializing the V8 environment, but make sure that errors or invalid flags fromget_v8_flags_from_envare properly handled or at least logged. Currently, any malformed flags could potentially cause unexpected runtime behavior or crashes.src/typegate/src/runtimes/deno/hooks/mod.ts (1)
67-67: Good practice specifying the error type.Marking the error as
anyclarifies its expected type, improving readability and helping the compiler. You may consider narrowing the error type (e.g.,unknown) to avoid propagating too broad a type. That said, this is acceptable given the dynamic nature of error handling here.src/typegate/src/runtimes/python/hooks/mod.ts (1)
57-57: Appropriate explicit typing for error.By defining the error parameter as
any, you acknowledge that Python code validation might yield unstructured exceptions. Consider using a more specific type if possible to expedite debugging, thoughanymay be necessary when dealing with multiple error shapes.tools/tree-view.ts (3)
18-21: Imports updated to reflect correct types.Switching from
TypeGraphDStoTypegraphand addingInjectionclarifies the intended structures and improves code correctness.
25-31: Refined function signature and usage of typed injection.Updating the
treeViewfunction’s parameter type toTypegraphand enforcing theInjectiontype ensures greater consistency and expressiveness in the codebase.
72-72: Maintain clarity in typed arrays.Using
Typegraph[]instead ofTypeGraphDS[]to match the updated data model avoids confusion and ensures correctness when iterating over or mapping this array.src/meta-cli/src/cli/typegate.rs (1)
71-76: Good addition of V8 flags initialization.
Invokinginit_v8_flagsensures that Deno's V8 engine respects environment-specific flags. This appears consistent with the rest of the changes updating Deno functionality in this PR.tools/deps.ts (1)
55-55: URL update looks fine.
Switching to theraw.githubusercontent.comURL fordeno-uddis consistent with the project standards for direct raw content links.tools/list-duplicates.ts (1)
15-15: Imports reorganized to includered.
It's good practice to maintain consistent import ordering for better readability. This addition clarifies usage ofredfor highlighting duplicate names.tools/consts.ts (1)
8-9: Great job on version synchronization!The Rust and Deno version constants have been updated to align with the PR objectives. This keeps the project’s versions consistent with the rest of the codebase.
src/typegate/src/services/graphql_service.ts (1)
55-59: Nice catch on consistent error naming!Renaming the caught error from
etoerrclarifies the error semantics. Good practice for easier debugging and maintainability.src/typegate/src/services/auth/mod.ts (1)
75-85: Ensureundefinedhandling forauth.Switching from
nulltoundefinedrequires careful handling of missing auth protocols. Check if downstream usage expectsnullinstead.src/xtask/src/deno.rs (2)
48-52: Initialize V8 flags early to avoid runtime conflicts.Calling
init_v8_flagswith environment flags is a solid step for stable performance. Confirm that these flags don't unexpectedly impact production builds or tests.
87-99: Harden import restrictions thoughtfully.Restricting imports by default is safer. Just ensure that required packages or domains aren’t accidentally blocked.
src/typegate/src/services/artifact_service.ts (1)
109-113: Uniform error handling looks good.The explicit check against
BaseErrorand fallback toUnknownErroris clear and maintains consistent error reporting for various error types.src/typegate/engine/src/lib.rs (1)
223-243: Ensure consistent test coverage for new permission flags.The changed permission flags in the test function now mirror the production code. Verify that tests are covering scenarios where reading, writing, and importing resources are involved, to ensure all permission flags work as intended.
src/typegate/src/services/auth/protocols/oauth2.ts (1)
167-167: Confirm usage of TypeScriptoverridemodifier
Usingoverrideclarifies that thisauthMiddlewaremethod is intentionally overriding a base class method, which helps detect accidental method name mismatches. This is a good practice for maintainability and clarity.src/typegate/src/runtimes/deno/deno.ts (1)
82-82: Consider robust error handling for secret retrieval
Currently, callingsecretManager.secretOrFail(secretName)will throw if the secret doesn't exist. To prevent partial initialization, consider wrapping this in a try block or logging additional context about which secret triggered the error.src/typegate/src/typegate/mod.ts (2)
202-203: Improved clarity for caught errors
Renaming the error variable toerrand typing it asanymakes it clear that the error could be of various types. This also aligns with consistent naming across the codebase.
206-211: Validate error types to prevent nested error possibilities
Iferr.errors[0]orerr.failureare missing or invalid, the code might throw a new runtime error. Consider additional checks or narrower type definitions to guard against unexpected error shapes.src/typegate/src/runtimes/typegate.ts (1)
337-337: Correct handling of nullish coalescing
Good fix! Adding parentheses ensures that the nullish coalescing operator correctly applies toq.modelNamebefore concatenation. The updated expression is now more precise and avoids potentialundefinedconcatenations.src/metagen-client-rs/src/graphql.rs (2)
372-374: Boxed response for request failures
This change adds flexibility to the response object. However, ensure the performance overhead of boxing is acceptable. In typical usage, it shouldn’t be a major concern.
421-421: Boxed GraphQLResponse
StoringGraphQLResponsein aBoxcan simplify the enum memory layout. Just confirm that any references to the original unboxed variant are updated accordingly.tools/Dockerfile (2)
56-57: LGTM: Installation method changeThe switch to using a shell script for GHJK installation is consistent with the changes in the workflow files.
2-3: Verify compatibility with updated versionsThe version updates align with the PR objectives. However, let's verify compatibility:
✅ Verification successful
Let me gather more information to verify the compatibility.
Let me gather more specific information about Rust and Deno version requirements.
Version updates are compatible with the codebase
The version updates in the Dockerfile (Rust 1.82.0 and Deno 2.1.2) are consistent with the versions defined in
tools/consts.tsand used throughout the codebase. These versions are:
- Used in the Dockerfile for building the container
- Referenced in the tooling configuration
- Used in the installation scripts via
tools/installs.tsNo incompatibilities or version conflicts were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any compatibility issues with the new versions # Check if Rust 1.82.0 is the minimum required version rg -A 2 "rust-version\s*=\s*\"1\." # Check for any explicit Deno version requirements rg -A 2 "deno\s+[0-9]"Length of output: 653
Script:
#!/bin/bash # Check Cargo.toml files for rust-version specification and dependencies fd Cargo.toml --exec cat {} # Check for any Deno configuration files fd deno.json --exec cat {} fd deno.lock --exec cat {} # Look for any Deno version constraints in TypeScript files rg "Deno.version"Length of output: 92101
Script:
#!/bin/bash # Check for Rust version requirements in build files and configs rg "rust-version|RUST_VERSION" -A 2 # Check for Deno version requirements in build files and configs rg "deno-version|DENO_VERSION" -A 2 # Check for any Deno version constraints in TypeScript files rg "Deno\.version|deno\s+[0-9]" -A 2 "*.ts"Length of output: 2349
Cargo.toml (2)
41-42: Minor version updates look goodThe updates to clap, clap_complete, and url dependencies are minor version bumps and should maintain backward compatibility.
Also applies to: 54-54
136-137: Verify deno upgrade impactThe upgrade from Deno v1.46.3 to v2.1.2 and deno_core from 0.307.0 to 0.323.0 is significant. Let's verify the changes:
✅ Verification successful
Based on the shell script results, I can now provide the final response:
Deno upgrade impact is minimal and safe
The codebase's usage of Deno APIs is limited to stable functionality that hasn't changed significantly between v1.46.3 and v2.1.2:
- Basic environment operations (
Deno.env)- File system operations (
Deno.mkdir,Deno.remove, etc.)- Network functionality (
Deno.serve,Deno.Command)- Standard utilities (
Deno.inspect)The deno_core usage is primarily for op definitions which remain compatible across the versions, with no usage of deprecated or removed APIs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential breaking changes # Check for Deno API usage that might be affected rg -A 3 "Deno\." src/ rg -A 3 "deno_core::" src/ # Check for any custom deno types ast-grep --pattern 'interface $name extends Deno.$rest { $$$ }'Length of output: 31996
.github/workflows/tests.yml (1)
32-32: LGTM: Added debug loggingAdding GHJK_LOG="debug" will help with troubleshooting CI issues.
.ghjk/lock.json (1)
734-744: Version updates look good!The updates align with the PR objectives:
- Added Deno v2.1.2 configuration
- Updated Rust to v1.82.0
Also applies to: 2259-2259
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (12)
tests/metagen/typegraphs/sample/py/client.py (5)
812-812: Consider adding docstrings to clarify the purpose of this method.The newly added
Post()method appears straightforward, but adding a brief docstring explaining its role in defining the schema would enhance maintainability and readability for future contributors.
967-975: Clarity and consistency: add docstrings to reflect theUserstructure.The
UserTypedDict has fields referencing specialized types (likeUserIdStringUuid). Consider adding docstrings or short comments about each field's usage or constraints, especially as the project evolves.
1044-1052: Initialization suggestions.The mapping
{"UserIdStringUuid": "String!", "PostSlugString": "String!", ...}is a minimal type-to-GraphQL mapping. If more types are added in the future, consider organizing them in a dedicated configuration or constants file to avoid repetition.
1063-1065: Check for symmetrical patterns inget_posts.This block mirrors
get_user. If you foresee more custom logic in the future (like complex filters), consider factoring out the common code into a helper function.
1089-1095: Use consistent naming across no-args calls.
composite_no_argsis fine. Should you have multiple similar no-argument functions, keep consistent naming to reduce confusion in usage..github/workflows/tests.yml (3)
32-32: Check potential verbosity fromGHJK_LOGdebug setting.While setting
GHJK_LOGtodebugaids troubleshooting, consider toggling it off or lowering verbosity outside of debug scenarios to prevent log clutter.
173-173: Remove trailing whitespace.A minor YAML lint issue: line 173 has trailing spaces. It’s best practice to remove them for a cleaner version history and to satisfy lint checks.
- +🧰 Tools
🪛 yamllint (1.35.1)
[error] 173-173: trailing spaces
(trailing-spaces)
284-284: Remove trailing whitespace.Line 284 contains trailing spaces. This can break certain lint or YAML validation rules.
- +🧰 Tools
🪛 yamllint (1.35.1)
[error] 284-284: trailing spaces
(trailing-spaces)
.github/workflows/release.yml (4)
37-37: Remove trailing whitespace.Line 37 has trailing spaces. Removing unnecessary whitespace promotes cleaner diffs.
- +🧰 Tools
🪛 yamllint (1.35.1)
[error] 37-37: trailing spaces
(trailing-spaces)
106-110: Consistent approach to ghjk setup.Repetitive block for installing
ghjk. If you have many workflows, consider centralizing this logic in a reusable Action or script to reduce duplication.
185-185: Remove trailing whitespace.Line 185 has trailing spaces. Clear them to maintain consistent formatting.
- +🧰 Tools
🪛 yamllint (1.35.1)
[error] 185-185: trailing spaces
(trailing-spaces)
324-324: Remove trailing whitespace.Line 324 has trailing spaces. Consider cleaning these up to stay consistent with linting rules.
- +🧰 Tools
🪛 yamllint (1.35.1)
[error] 324-324: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockdeno.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.github/workflows/release.yml(4 hunks).github/workflows/tests.yml(5 hunks)Cargo.toml(4 hunks)src/metagen-client-rs/Cargo.toml(1 hunks)tests/metagen/typegraphs/sample/py/client.py(3 hunks)tests/runtimes/deno/ts/math.ts(1 hunks)tools/Dockerfile.dockerignore(1 hunks)tools/list-duplicates.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/list-duplicates.ts
- Cargo.toml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/tests.yml
176-176: shellcheck reported issue in this script: SC2088:warning:1:7: Tilde does not expand in quotes. Use $HOME
(shellcheck)
176-176: shellcheck reported issue in this script: SC2086:info:1:24: Double quote to prevent globbing and word splitting
(shellcheck)
177-177: shellcheck reported issue in this script: SC2086:info:1:47: Double quote to prevent globbing and word splitting
(shellcheck)
289-289: shellcheck reported issue in this script: SC2088:warning:1:7: Tilde does not expand in quotes. Use $HOME
(shellcheck)
289-289: shellcheck reported issue in this script: SC2086:info:1:24: Double quote to prevent globbing and word splitting
(shellcheck)
290-290: shellcheck reported issue in this script: SC2086:info:1:47: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint (1.35.1)
.github/workflows/tests.yml
[error] 173-173: trailing spaces
(trailing-spaces)
[error] 284-284: trailing spaces
(trailing-spaces)
.github/workflows/release.yml
[error] 37-37: trailing spaces
(trailing-spaces)
[error] 185-185: trailing spaces
(trailing-spaces)
[error] 324-324: trailing spaces
(trailing-spaces)
🔇 Additional comments (25)
src/metagen-client-rs/Cargo.toml (2)
6-10: LGTM! Well-documented optimization strategyThe comments clearly explain the Docker cache optimization strategy, and the library configuration follows Rust conventions.
Line range hint
13-24: Verify dependency compatibility with Rust 1.82.0The dependencies appear to be using recent versions. Let's verify their compatibility with Rust 1.82.0.
✅ Verification successful
Let me gather more information to verify the compatibility.
Dependencies are compatible with Rust 1.82.0
Based on the verification results:
- All dependencies are using stable versions that are compatible with Rust 1.82.0
reqwest 0.12.9,serde 1.0.210, and other dependencies are recent releases from their respective stable branches- No MSRV (Minimum Supported Rust Version) constraints were found that would conflict with Rust 1.82.0
- Only one deprecated package was found in the dependency tree (
0.9.34+deprecated), but it's a transitive dependency and doesn't affect compatibilityThe dependency versions look good and should work well with Rust 1.82.0. No compatibility issues were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if dependencies are compatible with Rust 1.82.0 # Expected: No compatibility issues reported # Check for any known compatibility issues with these dependencies gh api graphql -f query=' { securityVulnerabilities(first: 10, ecosystem: RUST, package: "serde") { nodes { advisory { summary severity affectedVersions } } } }' # List any deprecated features or compatibility warnings rg -A 2 'deprecated|WARN' Cargo.lock || trueLength of output: 802
Script:
#!/bin/bash # Check Cargo.toml for MSRV (Minimum Supported Rust Version) specifications rg -A 2 'rust-version|MSRV' Cargo.toml # Check dependencies' versions in Cargo.lock rg -A 2 'name = "(serde|reqwest|mime_guess|futures|tokio-util|derive_more|lazy_static)"' Cargo.lock # Look for any compatibility warnings in the repository rg -i 'requires?.*(rust|version).*[0-9]+\.[0-9]+' . --type rustLength of output: 1267
tests/runtimes/deno/ts/math.ts (2)
4-5: Consider clarifying license usage for external code reference.
You reference an external code link in the file comments. Ensure compatibility with MPL-2.0 and add relevant attribution if required by the external project's license.
7-7: Verify pinned version of Big.js.
You're specifying Big.js at version 6.2.2, which may be outdated. Confirm that this version suits your needs and is free of known vulnerabilities or consider upgrading to the latest stable version for security patches and new features.✅ Verification successful
Big.js version 6.2.2 is the latest available version
The project is already using the latest stable version (6.2.2) of Big.js from npm registry, so no upgrade is necessary at this time.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for the latest version of Big.js on npm: curl -s "https://registry.npmjs.org/big.js" | jq '."dist-tags"'Length of output: 87
tests/metagen/typegraphs/sample/py/client.py (14)
940-940: Good practice: align naming conventions with usage context.Declaring
UserIdStringUuid = stris a quick way to alias astrtype. Ensure that this naming convention is consistent across the entire codebase and that each usage ofUserIdStringUuidremains self-explanatory.
945-953: TypedDict definitions look consistent.These new lines correctly define the
PostTypedDict with appropriate fields and type aliases. Although optional, consider adding docstrings or comments indicating the meaning of each field for improved clarity.
955-961: Keep input schema well-documented.
RootCompositeArgsFnInputis defined accurately. To avoid confusion, consider adding a comment or docstring clarifying its usage in method calls within the codebase (e.g., incomposite_args).
999-1008: Avoid potential confusion with naming.
PostSelectionsis a clear, descriptive name for a selection set. Overall, the typed definitions are consistent. If at any point the structure grows more complex, consider grouping these definitions in a dedicated module.
1010-1019: Good maintainability approach inUserSelections.This definition cleanly separates user field selections from
PostSelections. The code is well-structured and consistent with the rest of the typed selection blocks.
1021-1030: Successful union approach forRootCompositeUnionFnOutputSelections.The typed dict structure is well-defined, capturing union members that can refer to either
postorusersub-selections. This helps keep complex union logic cohesive.
1031-1039: Check for future expansion.
RootMixedUnionFnOutputSelectionsmerges multiple union possibilities. This is fine for current usage. If more variants appear in the future, ensure naming remains intuitive (e.g., rename if more complex types are introduced).
1055-1057: Validation checks forgetUserselection.The usage of
selection_to_nodesis correct, but remember to add unit tests to confirm that invalid or missing fields inselectraise proper exceptions.
1071-1073:scalarNoArgsusage is correct.No arguments are required, so this direct approach is fine. Be sure to cover it in tests verifying the query’s return value for typical and edge-cases.
1077-1085: Arg-based mutation logic.
scalar_argscan be expanded to use more specific typed dictionaries if needed. Currently, it’s properly referencingPostfor argument structure. Consider verifying the shape ofargsin a test suite.
1097-1109: Argument handling incomposite_argsis well structured.The typed input
RootCompositeArgsFnInputis used effectively. This keeps the system strongly typed. A thorough integration test would ensure that the typed dictionary and selection objects remain in sync.
1111-1119: Union logic checks.
scalar_unionmerges different types under a single query node. Remain vigilant about shape mismatches in arguments or selections. If necessary, add logic for safer runtime type checks.
1121-1133: Solid union pattern incomposite_union.This method properly handles union variants. If you plan to expand variants in the future, ensure compatibility with the selection transformations in
NodeDescs.
1135-1147: No issues inmixed_union.The code is consistent with the structure used in other union-based queries. Just ensure the project’s unit or integration tests validate the multiple union scenarios.
.github/workflows/tests.yml (4)
91-97: Shell steps to install ghjk appear correct.Replacing the
setup-ghjkaction with direct commands is valid. Just ensure secure handling of environment variables and that the installed version is pinned or verified for reproducible builds.
140-149: Repeat patterns in shell commands.These lines reuse the method of installing ghjk. Watch for any drift in version references. Confirm that all references to
$GHJK_VERSIONare consistent.
176-179: Shell path expansions need double quoting and $HOME usage.ShellCheck warns about tilde expansions within quotes. Use
$HOMEin place of~and wrap variables in quotes to avoid word splitting or glob expansion.- - run: echo "~/.local/bin" >> $GITHUB_PATH - - run: echo "BASH_ENV=~/.local/share/ghjk/env.sh" >> $GITHUB_ENV + - run: echo "$HOME/.local/bin" >> "$GITHUB_PATH" + - run: echo "BASH_ENV=$HOME/.local/share/ghjk/env.sh" >> "$GITHUB_ENV"🧰 Tools
🪛 actionlint (1.7.4)
176-176: shellcheck reported issue in this script: SC2088:warning:1:7: Tilde does not expand in quotes. Use $HOME
(shellcheck)
176-176: shellcheck reported issue in this script: SC2086:info:1:24: Double quote to prevent globbing and word splitting
(shellcheck)
177-177: shellcheck reported issue in this script: SC2086:info:1:47: Double quote to prevent globbing and word splitting
(shellcheck)
289-292: Duplicate shell script improvements.As above, use
$HOME/.local/binand quote variables to avoid expansions. This ensures consistency and satisfies ShellCheck and actionlint warnings.- - run: echo "~/.local/bin" >> $GITHUB_PATH - - run: echo "BASH_ENV=~/.local/share/ghjk/env.sh" >> $GITHUB_ENV + - run: echo "$HOME/.local/bin" >> "$GITHUB_PATH" + - run: echo "BASH_ENV=$HOME/.local/share/ghjk/env.sh" >> "$GITHUB_ENV"🧰 Tools
🪛 actionlint (1.7.4)
289-289: shellcheck reported issue in this script: SC2088:warning:1:7: Tilde does not expand in quotes. Use $HOME
(shellcheck)
289-289: shellcheck reported issue in this script: SC2086:info:1:24: Double quote to prevent globbing and word splitting
(shellcheck)
290-290: shellcheck reported issue in this script: SC2086:info:1:47: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/release.yml (3)
39-43: Installation steps match the rest of the workflows.Mirrors the pattern of manually installing
ghjkin place of the old action. Looks consistent. Confirm that the environment variables are handled in the same way as other workflows.
186-191: Proper shell steps, but watch quoting.Like previous comparisons, ensure
$HOMEis used instead of~to avoid ShellCheck SC2088. Also confirm that you double quote all expansions.
326-330: Installation consistency for ghjk.The pattern continues: cURL, add local bin to path, set
BASH_ENV, and runghjk envs cook. This is acceptable, but consider a centralized snippet if used across multiple workflows.
| @@ -9,6 +9,7 @@ | |||
| !src/metagen/Cargo.toml | |||
| !src/metagen/src/fdk_rust/static/Cargo.toml | |||
| !src/metagen/src/client_rs/static/Cargo.toml | |||
| !src/metagen-client-rs/Cargo.toml | |||
There was a problem hiding this comment.
💡 Codebase verification
Potential missing Cargo.toml inclusions in Dockerfile.dockerignore
Several Cargo.toml files in the src/ directory are not included in the .dockerignore file:
- src/common/Cargo.toml
- src/mt_deno/Cargo.toml
- src/substantial/Cargo.toml
- src/typegate/engine/Cargo.toml
- src/typegate/standalone/Cargo.toml
- src/typegraph/core/Cargo.toml
Additionally, there's an entry for a non-existent file:
- src/metagen/src/client_rs/static/Cargo.toml
While the addition of src/metagen-client-rs/Cargo.toml is correct, the inconsistency in including all Cargo.toml files might affect build context optimization.
🔗 Analysis chain
LGTM! Addition follows established patterns
The inclusion of src/metagen-client-rs/Cargo.toml aligns with the existing pattern of including specific Cargo.toml files for build context optimization.
Let's verify the consistency of Cargo.toml inclusions:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all Cargo.toml files in src/ are properly included in .dockerignore
# Expected: All Cargo.toml files in src/ should have corresponding entries in .dockerignore
# Find all Cargo.toml files in src/
echo "Cargo.toml files in src/:"
fd Cargo.toml src/
# Check .dockerignore entries
echo -e "\nCorresponding .dockerignore entries:"
rg '!src/.+Cargo\.toml' tools/Dockerfile.dockerignore
Length of output: 732
| export function maxAsync({ numbers: values }: MinInput): Promise<number> { | ||
| let maxValue = new Big(values[0]); | ||
| for (const value of values) { | ||
| if ((new Big(value)).gt(maxValue)) { | ||
| maxValue = value; | ||
| } | ||
| } | ||
| return Promise.resolve(Number(maxValue)); |
There was a problem hiding this comment.
Resolve type confusion and add boundary checks in maxAsync.
This function also mixes Big instances with number types and lacks safety checks for an empty array. Fixing these issues prevents unexpected runtime errors and aligns the logic with min.
Proposed changes:
export function maxAsync({ numbers: values }: MinInput): Promise<number> {
+ if (values.length === 0) {
+ return Promise.reject("Cannot compute max of an empty array");
+ }
let maxValue = new Big(values[0]);
for (const value of values) {
- if ((new Big(value)).gt(maxValue)) {
- maxValue = value;
+ const bigVal = new Big(value);
+ if (bigVal.gt(maxValue)) {
+ maxValue = bigVal;
}
}
return Promise.resolve(Number(maxValue));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function maxAsync({ numbers: values }: MinInput): Promise<number> { | |
| let maxValue = new Big(values[0]); | |
| for (const value of values) { | |
| if ((new Big(value)).gt(maxValue)) { | |
| maxValue = value; | |
| } | |
| } | |
| return Promise.resolve(Number(maxValue)); | |
| export function maxAsync({ numbers: values }: MinInput): Promise<number> { | |
| if (values.length === 0) { | |
| return Promise.reject("Cannot compute max of an empty array"); | |
| } | |
| let maxValue = new Big(values[0]); | |
| for (const value of values) { | |
| const bigVal = new Big(value); | |
| if (bigVal.gt(maxValue)) { | |
| maxValue = bigVal; | |
| } | |
| } | |
| return Promise.resolve(Number(maxValue)); | |
| } |
| export function min({ numbers: values }: MinInput): number { | ||
| let minValue = new Big(values[0]); | ||
| for (const value of values) { | ||
| if ((new Big(value)).lt(minValue)) { | ||
| minValue = value; | ||
| } | ||
| } | ||
| return Number(minValue); |
There was a problem hiding this comment.
Fix inconsistent usage of Big type and handle empty arrays.
Within the min function, you assign minValue to a Big instance initially, then switch to a plain number. Also, there's no guard for an empty array. These issues may result in type confusion and potential runtime errors.
Consider applying this refactor:
export function min({ numbers: values }: MinInput): number {
+ if (values.length === 0) {
+ throw new Error("Cannot compute min of an empty array");
+ }
let minValue = new Big(values[0]);
for (const value of values) {
- if ((new Big(value)).lt(minValue)) {
- minValue = value;
+ const bigVal = new Big(value);
+ if (bigVal.lt(minValue)) {
+ minValue = bigVal;
}
}
return Number(minValue);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function min({ numbers: values }: MinInput): number { | |
| let minValue = new Big(values[0]); | |
| for (const value of values) { | |
| if ((new Big(value)).lt(minValue)) { | |
| minValue = value; | |
| } | |
| } | |
| return Number(minValue); | |
| export function min({ numbers: values }: MinInput): number { | |
| if (values.length === 0) { | |
| throw new Error("Cannot compute min of an empty array"); | |
| } | |
| let minValue = new Big(values[0]); | |
| for (const value of values) { | |
| const bigVal = new Big(value); | |
| if (bigVal.lt(minValue)) { | |
| minValue = bigVal; | |
| } | |
| } | |
| return Number(minValue); | |
| } |
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the updated release notes:
New Features
Dependencies
Improvements
Chores
Bug Fixes