-
Notifications
You must be signed in to change notification settings - Fork 13
chore: update deno to v2.2.4 #923
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
6f46266
to
15d7cbe
Compare
📝 WalkthroughWalkthroughThis set of changes introduces a new unified error type ( Changes
Sequence Diagram(s)sequenceDiagram
participant TS as TypeScript Layer
participant Rust as Rust Hostcall
participant OpErr as OpErr Error Type
TS->>Rust: Call hostcall (e.g., op_prisma_query)
Rust->>Rust: Execute operation
alt Success
Rust-->>TS: Return Result<T, OpErr> (Ok)
else Error
Rust->>OpErr: Map error to OpErr
Rust-->>TS: Return Result<T, OpErr> (Err(OpErr))
end
TS->>TS: catch (err: any) { ... }
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (49)
src/mt_deno/src/lib.rs (2)
246-252
: **Performing type checks **Calling
check_specifiers
for type validation is an excellent step. Consider whether partial failures or concurrency might need explicit handling or more granular error reporting.
272-273
: **Passingdesc_parser
to test executor **Passing the
desc_parser
is 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
any
clarifies 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 thanany
in the catch block.While this approach works, using
unknown
orError
(when certain) can improve type safety. Consider narrowing or casting the error instead of relying on theany
type.src/typegate/src/services/auth/protocols/jwt.ts (1)
60-67
: Avoid usingany
for error handling.Using
unknown
or 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 asany
is 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.injection
asany
is 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.injection
has 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 usingunknown
instead ofany
.While specifying a type for the caught error is an improvement over not specifying one at all, using
unknown
in place ofany
can 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
e
toerr
promotes clarity in error handling. However, consider usingunknown
instead ofany
for stronger type safety in TypeScript. Beyond this, the multiple checks forBaseError
,ResolverError
, andBadContext
remain 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 usingunknown
could 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_import
entries 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
any
sacrifices type safety. Consider usingunknown
and 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
any
is generally discouraged; usingunknown
and then narrowing or type-guarding may improve error handling reliability.
142-142
: Improve error typing.Again, consider using
unknown
to 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
any
forfeits type safety. Preferunknown
and then perform runtime checks or cast to a known error interface.
84-84
: Enhance error type usage in catch.Using
any
can hide actual error structures. Switch tounknown
and handle possible error cases explicitly.
108-108
: Encourage explicit error typing.Relying on
any
leads to potential maintainability issues. If possible, replace withunknown
and narrow the type.
127-127
: Improve error typing for better clarity.Switch from
any
tounknown
or a known error type to leverage TypeScript’s type checks.
149-149
: Refine error handling approach.Catching an
any
type can conceal important details; preferunknown
plus a type guard or instance check.
171-171
: Strengthen type safety in error handling.Change
any
tounknown
for stricter compile-time checks, reducing potential runtime errors.
190-190
: Favor narrower error types.Catching errors as
any
may hide critical type specifics, consider usingunknown
.
211-211
: Adopt more explicit error type.Maintain code clarity by dropping
any
in favor ofunknown
or a specialized error type.
234-234
: Increase error-type clarity.Use
unknown
to encourage explicit checking or transformation of caught errors.
253-253
: Use explicit error handling strategy.Replace
any
withunknown
, forcing callers to handle error details properly.
275-275
: Refine catch error typing.A narrower type than
any
helps ensure that error properties are validated before use.
297-302
: Leverage rigorous error typing.Both catch blocks here use
any
. Reviewing them forunknown
usage can improve clarity and safety.
339-339
: Favor safer error typing.Handling errors as
unknown
encourages robust validation and prevents accidental reliance on undefined properties.
362-362
: Improve error stability with narrower types.Using
unknown
ensures that reading properties on the error is done in a controlled manner.
388-388
: Enhance reliability with explicit error typing.Switching from
any
tounknown
can help avoid unexpected runtime failures.
412-412
: Strengthen error safety.
unknown
is preferable overany
to guarantee proper type checks for errors.
431-431
: Encourage a typed catch block.Use
unknown
in place ofany
so 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
any
tounknown
to 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
any
forunknown
.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 asany
is 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 whetherany
is 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_VERSION
variable 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
any
reduces type safety. Consider usingunknown
for 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.lock
is excluded by!**/*.lock
deno.lock
is 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
PermissionsOptions
toargs::PermissionFlags
aligns 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
permissions
object toargs::Flags
is coherent with the refactored permission system. No issues noted.
235-235
: **Validate condition for empty test modules **The newly added
permit_no_files
check 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
Permissions
object prevents unintended carry-over of granted or revoked permissions between tests. This design choice appears robust.
268-268
: **Running test specifiers **Invoking
test_specifiers
after 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 lint
command **Executing
deno lint
afterdeno check
ensures 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
existsSync
import 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_flags
is beneficial for properly initializing the V8 environment, but make sure that errors or invalid flags fromget_v8_flags_from_env
are 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
any
clarifies 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, thoughany
may be necessary when dealing with multiple error shapes.tools/tree-view.ts (3)
18-21
: Imports updated to reflect correct types.Switching from
TypeGraphDS
toTypegraph
and addingInjection
clarifies the intended structures and improves code correctness.
25-31
: Refined function signature and usage of typed injection.Updating the
treeView
function’s parameter type toTypegraph
and enforcing theInjection
type 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_flags
ensures 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.com
URL fordeno-udd
is 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 ofred
for 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
e
toerr
clarifies the error semantics. Good practice for easier debugging and maintainability.src/typegate/src/services/auth/mod.ts (1)
75-85
: Ensureundefined
handling forauth
.Switching from
null
toundefined
requires careful handling of missing auth protocols. Check if downstream usage expectsnull
instead.src/xtask/src/deno.rs (2)
48-52
: Initialize V8 flags early to avoid runtime conflicts.Calling
init_v8_flags
with 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
BaseError
and fallback toUnknownError
is 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 TypeScriptoverride
modifier
Usingoverride
clarifies that thisauthMiddleware
method 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 toerr
and typing it asany
makes 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.failure
are 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.modelName
before concatenation. The updated expression is now more precise and avoids potentialundefined
concatenations.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
StoringGraphQLResponse
in aBox
can 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.ts
and 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.ts
No 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theUser
structure.The
User
TypedDict 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_args
is 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_LOG
debug setting.While setting
GHJK_LOG
todebug
aids 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.lock
is excluded by!**/*.lock
deno.lock
is 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 = str
is a quick way to alias astr
type. Ensure that this naming convention is consistent across the entire codebase and that each usage ofUserIdStringUuid
remains self-explanatory.
945-953
: TypedDict definitions look consistent.These new lines correctly define the
Post
TypedDict 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.
RootCompositeArgsFnInput
is 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.
PostSelections
is 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
post
oruser
sub-selections. This helps keep complex union logic cohesive.
1031-1039
: Check for future expansion.
RootMixedUnionFnOutputSelections
merges 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 forgetUser
selection.The usage of
selection_to_nodes
is correct, but remember to add unit tests to confirm that invalid or missing fields inselect
raise proper exceptions.
1071-1073
:scalarNoArgs
usage 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_args
can be expanded to use more specific typed dictionaries if needed. Currently, it’s properly referencingPost
for argument structure. Consider verifying the shape ofargs
in a test suite.
1097-1109
: Argument handling incomposite_args
is well structured.The typed input
RootCompositeArgsFnInput
is 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_union
merges 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-ghjk
action 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_VERSION
are consistent.
176-179
: Shell path expansions need double quoting and $HOME usage.ShellCheck warns about tilde expansions within quotes. Use
$HOME
in 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/bin
and 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
ghjk
in 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
$HOME
is 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
tools/list-duplicates.ts (1)
Line range hint
81-104
: Remove commented-out code.Instead of keeping the old file-based input code commented out, it should be removed. If needed, the code can be retrieved from version control history.
♻️ Duplicate comments (1)
.github/workflows/tests.yml (1)
289-291
:⚠️ Potential issueFix shell script security issues.
Same issues as found in the test-website job.
🧰 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)
🧹 Nitpick comments (3)
src/typegate/src/runtimes/s3.ts (1)
96-96
: Prefer explicit S3 object type overany
.While using
any
might quickly silence type complaints, it undermines the benefits of static typing. If the AWS SDK provides a specific shape forres.Contents
, please consider using that shape (for example,Object
orObjectList
fromaws-sdk/client-s3
) to ensure you have correct types across the codebase.-keys: (res.Contents ?? []).map((c: any) => ({ +keys: (res.Contents ?? []).map((c: Object) => ({ key: c.Key, size: c.Size, }))tools/list-duplicates.ts (2)
22-23
: Consider encapsulating counters in a result object.Using global mutable state can make the code harder to test and debug. Consider refactoring to return these metrics as part of a result object from
listDuplicates
.-let whereTypeCount = 0; -let optionalTypeCount = 0; +interface DuplicateAnalysis { + whereTypeCount: number; + optionalTypeCount: number; + duplicates: Map<string, readonly [number, TypeNode][]>; +} + -export function listDuplicates(tg: TypeGraphDS, rootIdx = 0) { +export function listDuplicates(tg: TypeGraphDS, rootIdx = 0): DuplicateAnalysis { + let whereTypeCount = 0; + let optionalTypeCount = 0; // ... existing code ... + return { + whereTypeCount, + optionalTypeCount, + duplicates: bins + }; }
50-51
: Consider extracting duplicate injection handling logic.The injection handling code is duplicated. Consider extracting it into a helper function for better maintainability.
+function formatInjection(type: TypeNode): string { + // deno-lint-ignore no-explicit-any + return "injection" in type ? ` (injection ${(type.injection as any).source})` : ""; +} // Then use it in both places: -const injection = "injection" in type - // deno-lint-ignore no-explicit-any - ? ` (injection ${(type.injection as any).source})` - : ""; +const injection = formatInjection(type);Also applies to: 64-65
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/release.yml
(5 hunks).github/workflows/tests.yml
(5 hunks)src/meta-cli/src/macros.rs
(1 hunks)src/typegate/src/runtimes/s3.ts
(1 hunks)tools/list-duplicates.ts
(4 hunks)
🧰 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)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: bulid-docker (linux/arm64, custom-arm)
- GitHub Check: test-website
- GitHub Check: test-full
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: pre-commit
🔇 Additional comments (6)
src/meta-cli/src/macros.rs (1)
13-17
: Confirm the'static
trait object usage for concurrency and error chaining.This update properly boxes the error as a trait object that is both
Send
andSync
, which is beneficial when errors need to be passed across threads. However, confirm that forcing'static
is necessary and doesn’t accidentally cut off useful error context or references. If this accommodation is intended for concurrency or error propagation, then the change is sound. Otherwise, consider whether'static
is strictly required.tools/list-duplicates.ts (2)
8-8
: LGTM! Usage documentation updated correctly.The usage comment and import organization accurately reflect the changes in input handling method.
Also applies to: 15-15
30-42
: LGTM! Type processing logic is sound.The implementation correctly:
- Handles type structure hashing
- Tracks duplicate names consistently
- Counts specific type patterns
.github/workflows/tests.yml (2)
176-178
:⚠️ Potential issueFix shell script security issues.
The commands need proper quoting and should use $HOME instead of tilde.
Apply this fix:
- - 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"Likely invalid or redundant comment.
🧰 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)
Line range hint
61-61
: Verify version tagging strategy change.The prerelease condition now checks for 'rc' instead of previous version tags.
Let's check the existing tags to ensure this change aligns with the project's versioning strategy:
✅ Verification successful
I don't see any prerelease condition checking for 'rc' or 'alpha' in the provided workflow file. Let me check other workflow files that might contain version tagging logic.
I found the relevant prerelease condition in the release.yml workflow file. The condition is used in the
check-bump
job to determine if a release should be marked as prerelease:prerelease: ${{ contains(steps.latest-tag.outputs.tag, 'dev') || contains(steps.latest-tag.outputs.tag, 'rc') }}Let me check if there are any other files that might be affected by this version tagging strategy.
Let me try a different search approach without file type restrictions.
Let me check the actual content of these files to see if they're related to version tagging.
Based on the analysis of the workflow files and codebase, I can now provide a final response:
Version tagging strategy change is aligned with the project's conventions
The change from 'alpha' to 'rc' in the prerelease condition is consistent with the project's versioning strategy. The release workflow (.github/workflows/release.yml) already handles both 'rc' and 'dev' tags as prereleases:
prerelease: ${{ contains(steps.latest-tag.outputs.tag, 'dev') || contains(steps.latest-tag.outputs.tag, 'rc') }}This change appears to be an intentional shift from alpha to release candidate (rc) versioning, which is a common progression in semantic versioning.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check existing version tags git tag -l "v*" | grep -E 'alpha|rc|dev'Length of output: 42
Script:
#!/bin/bash # Let's check the workflow file to understand the version tagging logic cat .github/workflows/tests.ymlLength of output: 12203
Script:
#!/bin/bash # Let's check all workflow files for version tagging logic fd -e yml -e yaml . .github/workflows/ --exec cat {}Length of output: 31379
Script:
#!/bin/bash # Search for files containing version tag related patterns rg -l "alpha|rc|dev" --type yaml --type rust --type typescriptLength of output: 99
Script:
#!/bin/bash # Search for files containing version tag related patterns rg -l "alpha|rc|dev" --type yamlLength of output: 670
Script:
#!/bin/bash # Check content of relevant workflow and configuration files echo "=== .gitlab-ci.yml ===" cat .gitlab-ci.yml echo -e "\n=== whiz.yaml ===" cat whiz.yamlLength of output: 1862
.github/workflows/release.yml (1)
37-43
: LGTM! Consistent and secure installation commands.All instances of the ghjk installation commands are properly implemented with correct quoting and environment variable usage.
Also applies to: 106-110, 185-191, 324-330
🧰 Tools
🪛 yamllint (1.35.1)
[error] 37-37: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (21)
tests/metagen/typegraphs/sample/rs_upload/Cargo.toml (1)
7-7
: Changes are consistent across test packages.The dependency change matches the one in
sample_client/Cargo.toml
, maintaining consistency. However, please:
- Document the rationale for moving away from workspace dependencies
- Consider adding a comment in both Cargo.toml files explaining this setup
tests/tools/schema_test.ts (2)
4-6
: Consider using import_map.json for version management.While directly specifying versions in URLs works, consider moving these version specifications to an import map for better maintainability.
32-39
: Consider enhancing error messages with validation details.When validation fails, include the specific validation errors from Ajv to help developers quickly identify and fix issues.
- assert(result, `validation failed for '${file}'`); + if (!result) { + assert(false, `Validation failed for '${file}': ${JSON.stringify(validate.errors, null, 2)}`); + }tests/metagen/typegraphs/sample.ts (1)
47-56
: Consider adding JSDoc comments for the nested structure.The nestedComposite structure is well-designed, but documentation would help explain its purpose and usage.
+/** + * Represents a nested structure with scalar, composite, and list fields. + * @property {number} scalar - An integer value + * @property {object} composite - A nested structure containing value and further nesting + * @property {Array<{value: number}>} list - A list of objects with integer values + */ const nestedComposite = t.struct({ scalar: t.integer(), composite: t.struct({tests/metagen/typegraphs/sample/rs/client.rs (2)
268-287
: Consider adding documentation for the partial structures.While the implementation is correct, adding documentation comments would help explain:
- The purpose of each partial structure
- The relationship between these structures
- Usage examples
Apply this diff to add documentation:
+/// Represents the nested structure within a composite structure #[derive(Debug, serde::Serialize, serde::Deserialize)] pub struct RootNestedCompositeFnOutputCompositeStructNestedStructPartial { pub inner: Option<RootScalarUnionFnOutputT1Integer>, } +/// Represents a composite structure that can contain nested structures #[derive(Debug, serde::Serialize, serde::Deserialize)] pub struct RootNestedCompositeFnOutputCompositeStructPartial { pub value: Option<RootScalarUnionFnOutputT1Integer>, pub nested: Option<RootNestedCompositeFnOutputCompositeStructNestedStructPartial>, }
315-337
: Consider adding derive macros for common traits.The selection structures could benefit from implementing additional traits for better ergonomics.
Apply this diff to add common traits:
-#[derive(Default, Debug)] +#[derive(Default, Debug, Clone, PartialEq)] pub struct RootNestedCompositeFnOutputCompositeStructNestedStructSelections<ATy = NoAlias> { pub inner: ScalarSelect<ATy>, }src/metagen/src/client_py/static/client.py (1)
20-25
: Improve boolean expression readabilityThe boolean expression can be simplified using the built-in
bool()
function.- select_all = True if sub_flags is not None and sub_flags.select_all else False + select_all = bool(sub_flags is not None and sub_flags.select_all)🧰 Tools
🪛 Ruff (0.8.2)
25-25: Use
bool(...)
instead ofTrue if ... else False
Replace with `bool(...)
(SIM210)
tests/metagen/typegraphs/sample/py_upload/client.py (1)
23-28
: Improve boolean expression readabilitySimilar to the previous file, the boolean expression can be simplified.
- select_all = True if sub_flags is not None and sub_flags.select_all else False + select_all = bool(sub_flags is not None and sub_flags.select_all)🧰 Tools
🪛 Ruff (0.8.2)
28-28: Use
bool(...)
instead ofTrue if ... else False
Replace with `bool(...)
(SIM210)
tools/list-duplicates.ts (5)
21-22
: Consider using a more structured approach for counters.The global variables could lead to issues with state management, especially when processing multiple type graphs.
Consider encapsulating the counters in a class or returning them as part of the function result:
-let whereTypeCount = 0; -let optionalTypeCount = 0; +interface TypeCounts { + whereTypeCount: number; + optionalTypeCount: number; +} + +class TypeCounter { + private whereTypeCount = 0; + private optionalTypeCount = 0; + + increment(type: 'where' | 'optional'): void { + if (type === 'where') this.whereTypeCount++; + else this.optionalTypeCount++; + } + + getCounts(): TypeCounts { + return { + whereTypeCount: this.whereTypeCount, + optionalTypeCount: this.optionalTypeCount + }; + } +}
23-41
: Improve selection flag handling.The selection flag handling could be more robust with better type checking and error messages.
Consider these improvements:
- sub_flags = selection.get("_") - if sub_flags is not None and not isinstance(sub_flags, SelectionFlags): + const sub_flags = selection.get("_"); + if (sub_flags !== undefined && !(sub_flags instanceof SelectionFlags)) { throw new Error( - f"selection field '_' should be of type SelectionFlags but found {type(sub_flags)}" + `selection field '_' should be of type SelectionFlags but found ${typeof sub_flags}` ); }
28-28
: Replace conditional with direct boolean conversion.The boolean conversion can be simplified.
- select_all = True if sub_flags is not None and sub_flags.select_all else False + select_all = Boolean(sub_flags?.select_all)
36-41
: Consider using a more descriptive pattern for type matching.The pattern matching for
_where_
could be more specific to avoid false positives.- if (title.match(/_where_/)) { + if (title.match(/^.*_where_.*$/)) { whereTypeCount++; } - if (type.type == "optional") { + if (type.type === "optional") { optionalTypeCount++; }
117-117
: Enhance counter logging with more context.The counter logging could be more informative.
-console.log({ whereTypeCount, optionalTypeCount }); +console.log({ + stats: { + whereTypeCount, + optionalTypeCount, + }, + message: 'Type counting complete' +});tools/schema/metatype.json (4)
1-6
: Add schema metadata for better discoverability.The schema could benefit from additional metadata fields.
{ "$schema": "http://json-schema.org/draft-07/schema#", "$id": "https://json.schemastore.org/metatype.json", "title": "Metatype configuration file schema", + "description": "Configuration schema for Metatype framework", + "type": "object", + "$comment": "Schema version: 1.0.0", "additionalProperties": false, - "type": "object",
204-208
: Strengthen URL validation pattern.The URL format validation could be more specific.
"url": { "type": "string", "format": "uri", + "pattern": "^https?://[\\w\\-\\.]+(:\\d+)?(/.*)?$", "description": "The base URL of the typegate server, example: <http://localhost:7890>." },
221-227
: Add pattern validation for environment variables.Environment variables should follow a standard naming convention.
"env": { "type": "object", "description": "Environment variables for the typegate.", "additionalProperties": { - "type": "string" + "type": "string", + "pattern": "^[A-Z][A-Z0-9_]*$" } },
268-271
: Add path validation for migrations directory.The migrations path should follow standard path conventions.
"migrations_path": { "type": "string", + "pattern": "^[\\w\\-\\.\\/_]+$", "description": "The directory for storing Prisma migration files." }
tests/metagen/metagen_test.ts (3)
37-37
: Consider using a custom logger for S3 operations.Using console directly for S3 client logging could mix operational logs with test output.
- logger: console, + logger: { + debug: (...args) => console.debug('[S3]', ...args), + info: (...args) => console.info('[S3]', ...args), + warn: (...args) => console.warn('[S3]', ...args), + error: (...args) => console.error('[S3]', ...args) + },
42-45
: Improve error handling in bucket preparation.The error handling for bucket creation could be more specific.
- } catch (err: any) { - if (err?.Code != "BucketAlreadyOwnedByYou") { - throw err; - } + } catch (err) { + if (err instanceof Error) { + if ('Code' in err && err.Code === 'BucketAlreadyOwnedByYou') { + console.info('Bucket already exists and is owned by you'); + return; + } + throw new Error(`Failed to create bucket: ${err.message}`); + } + throw err;
598-615
: Add validation for nested composite schema.The schema definition for nested composites could benefit from additional validation.
const expectedSchemaC = zod.object({ scalarOnly: zod.object({ scalar: zod.number() }), withStruct: zod.object({ scalar: zod.number(), - composite: zod.object({ value: zod.number() }), + composite: zod.object({ + value: zod.number().min(0), + description: zod.string().optional() + }), }), withStructNested: zod.object({ scalar: zod.number(), composite: zod.object({ - value: zod.number(), + value: zod.number().min(0), nested: zod.object({ inner: zod.number() }), }), }), withList: zod.object({ scalar: zod.number(), - list: zod.array(zod.object({ value: zod.number() })), + list: zod.array(zod.object({ + value: zod.number().min(0), + description: zod.string().optional() + })).min(1), }), });tests/metagen/typegraphs/sample/py/client.py (1)
23-28
: Simplify boolean expression and improve variable naming.The variable rename from
flags
tosub_flags
improves clarity. However, the boolean expression can be simplified.Apply this diff to simplify the code:
- select_all = True if sub_flags is not None and sub_flags.select_all else False + select_all = bool(sub_flags is not None and sub_flags.select_all)🧰 Tools
🪛 Ruff (0.8.2)
28-28: Use
bool(...)
instead ofTrue if ... else False
Replace with `bool(...)
(SIM210)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
.ghjk/deno.lock
is excluded by!**/*.lock
Cargo.lock
is excluded by!**/*.lock
deno.lock
is excluded by!**/*.lock
tests/metagen/typegraphs/sample/rs/Cargo.lock
is excluded by!**/*.lock
tests/metagen/typegraphs/sample/rs_upload/Cargo.lock
is excluded by!**/*.lock
tests/runtimes/wasm_reflected/rust/Cargo.lock
is excluded by!**/*.lock
📒 Files selected for processing (40)
.ghjk/lock.json
(14 hunks)examples/templates/deno/api/example.ts
(1 hunks)examples/typegraphs/metagen/rs/fdk.rs
(1 hunks)ghjk.ts
(1 hunks)import_map.json
(1 hunks)src/metagen-client-rs/src/selection.rs
(1 hunks)src/metagen/src/client_py/static/client.py
(3 hunks)src/metagen/src/client_ts/static/mod.ts
(1 hunks)src/substantial/Cargo.toml
(1 hunks)src/substantial/src/protocol/events.rs
(2 hunks)src/substantial/src/protocol/metadata.rs
(2 hunks)src/typegate/src/runtimes/deno/deno_messenger.ts
(1 hunks)src/typegate/src/runtimes/deno/hooks/mod.ts
(4 hunks)src/typegate/src/runtimes/substantial/workflow_worker_manager.ts
(1 hunks)src/typegate/src/typegate/hooks.ts
(2 hunks)src/typegate/src/typegate/mod.ts
(1 hunks)tests/e2e/published/published_test.ts
(4 hunks)tests/injection/random_injection_test.ts
(1 hunks)tests/metagen/metagen_test.ts
(3 hunks)tests/metagen/typegraphs/identities/rs/fdk.rs
(1 hunks)tests/metagen/typegraphs/sample.ts
(1 hunks)tests/metagen/typegraphs/sample/py/client.py
(6 hunks)tests/metagen/typegraphs/sample/py/main.py
(1 hunks)tests/metagen/typegraphs/sample/py_upload/client.py
(3 hunks)tests/metagen/typegraphs/sample/rs/Cargo.toml
(1 hunks)tests/metagen/typegraphs/sample/rs/client.rs
(4 hunks)tests/metagen/typegraphs/sample/rs/main.rs
(2 hunks)tests/metagen/typegraphs/sample/rs_upload/Cargo.toml
(1 hunks)tests/metagen/typegraphs/sample/ts/client.ts
(5 hunks)tests/metagen/typegraphs/sample/ts/main.ts
(2 hunks)tests/metagen/typegraphs/sample/ts_upload/client.ts
(1 hunks)tests/runtimes/deno/deno_sync_test.ts
(1 hunks)tests/runtimes/wasm_wire/rust/fdk.rs
(1 hunks)tests/tools/schema_test.ts
(1 hunks)tests/utils/process.ts
(1 hunks)tools/installs.ts
(1 hunks)tools/list-duplicates.ts
(4 hunks)tools/schema/metatype.json
(1 hunks)tools/tasks/gen.ts
(1 hunks)tools/tasks/lint.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- src/substantial/Cargo.toml
- examples/templates/deno/api/example.ts
- tests/metagen/typegraphs/identities/rs/fdk.rs
- src/substantial/src/protocol/metadata.rs
- src/substantial/src/protocol/events.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/typegate/src/runtimes/deno/deno_messenger.ts
- src/typegate/src/typegate/mod.ts
- ghjk.ts
- tools/tasks/lint.ts
- src/typegate/src/runtimes/deno/hooks/mod.ts
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/metagen/typegraphs/sample/py_upload/client.py
28-28: Use bool(...)
instead of True if ... else False
Replace with `bool(...)
(SIM210)
src/metagen/src/client_py/static/client.py
25-25: Use bool(...)
instead of True if ... else False
Replace with `bool(...)
(SIM210)
tests/metagen/typegraphs/sample/py/client.py
28-28: Use bool(...)
instead of True if ... else False
Replace with `bool(...)
(SIM210)
🔇 Additional comments (38)
tools/tasks/gen.ts (2)
22-22
: Verify protobuf-codegen version compatibilityThe version update to 3.7.1 aligns with the changes in other files. Let's verify this is the latest compatible version.
✅ Verification successful
✓ Protobuf version 3.7.1 is consistently configured
The version update to 3.7.1 is correctly aligned between the Rust crate dependency and the TypeScript tooling configuration, with no conflicting versions found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check latest version of protobuf-codegen cargo search protobuf-codegen --limit 1 # Check if this version is used consistently rg -l "protobuf.*3\.7\.1"Length of output: 291
Script:
#!/bin/bash # Check the Cargo.toml content cat src/substantial/Cargo.toml # Look for any other protobuf version references rg -i "protobuf.*version" --type toml --type typescriptLength of output: 401
30-31
: Verify protoc command changesThe switch from
--rust_out
to--rs_out
and removal of experimental options represents a significant change in the protocol buffer code generation approach.Could you confirm if:
- The
--rs_out
flag is the new recommended approach?- The removed experimental options are no longer needed?
✅ Verification successful
Protoc command changes are correct and intentional ✅
The switch to
--rs_out
is the correct approach as confirmed by:
- Generated files headers showing rust-protobuf 3.7.1 using
--rs_out
- Matching protobuf-codegen v3.7.1 installation in the build task
- Successfully generated protocol files in the codebase
The removal of experimental options appears intentional and working correctly with the current toolchain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check protoc version and help protoc --version protoc --help | grep -A 5 "rs_out" # Look for any documentation or comments about this change rg -l "rust_out|rs_out" .Length of output: 380
Script:
#!/bin/bash # Check if these are generated files and look for any documentation cat src/substantial/src/protocol/metadata.rs | head -n 10 cat src/substantial/src/protocol/events.rs | head -n 10 # Look for documentation about protoc in markdown files fd -e md | xargs rg -l "protoc|protocol buffer" # Check the gen.ts file for comments about the change cat tools/tasks/gen.tsLength of output: 1870
tests/metagen/typegraphs/sample/rs/Cargo.toml (1)
7-7
: Verify workspace configuration impact.Switching from workspace to path dependency could lead to version inconsistencies. Please verify:
- Why the workspace dependency is being removed
- If this change is necessary for the Rust 1.82.0 upgrade
✅ Verification successful
Path dependency is correct due to workspace configuration
The change from workspace to path dependency is necessary because the test directory is explicitly excluded from the workspace in the root Cargo.toml. Both test packages consistently use path dependencies as they cannot use workspace dependencies when excluded.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the repository has a workspace configuration fd -t f "Cargo.toml" -x grep -l "workspace.members" # Check if metagen-client-rs is referenced in other Cargo.toml files fd -t f "Cargo.toml" -x grep -l "metagen-client"Length of output: 247
Script:
#!/bin/bash # Check root Cargo.toml content echo "=== Root Cargo.toml ===" cat Cargo.toml # Check how metagen-client is referenced in other files echo -e "\n=== src/metagen-client-rs/Cargo.toml ===" cat src/metagen-client-rs/Cargo.toml echo -e "\n=== tests/metagen/typegraphs/sample/rs_upload/Cargo.toml ===" cat tests/metagen/typegraphs/sample/rs_upload/Cargo.tomlLength of output: 7859
src/typegate/src/typegate/hooks.ts (1)
7-7
: LGTM!The import statement is properly typed and follows TypeScript conventions.
tools/installs.ts (1)
12-17
: LGTM! Comprehensive Rust configuration for WASM support.The enhanced Rust configuration properly includes the necessary components and targets for WASM compilation.
Let's verify the Rust configuration:
✅ Verification successful
Configuration verified: Rust setup properly supports WASM compilation
The configuration in
tools/installs.ts
correctly aligns with:
- Project's rust-toolchain.toml configuration
- Multiple WASM build scripts and requirements
- Documentation and usage patterns across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Rust configuration and WASM targets # Test: Check if rust-src component and WASM targets are available in the current Rust installation rustup component list | grep "rust-src" rustup target list | grep -E "wasm32-(unknown-unknown|wasi)"Length of output: 296
Script:
#!/bin/bash # Search for WASM and Rust-related configurations echo "=== Searching for WASM targets usage ===" rg -i "wasm32-(unknown-unknown|wasi)" -A 2 -B 2 echo -e "\n=== Searching for rust-src component references ===" rg -i "rust-src" -A 2 -B 2 echo -e "\n=== Looking for Rust build configurations ===" fd -e toml -e cargo --exec cat {}Length of output: 31199
tests/metagen/typegraphs/sample.ts (1)
25-32
: LGTM! Well-structured type definitions.The reformatted struct definitions improve readability while maintaining functionality.
Also applies to: 34-41
tests/metagen/typegraphs/sample/ts/main.ts (2)
25-48
: LGTM! Well-structured mutation preparation.The reformatted mutation preparation improves readability with clear type annotations and argument handling.
134-147
: LGTM! Comprehensive test coverage for nested structures.The new query thoroughly tests all aspects of the nestedComposite structure, including scalar fields, nested structs, and lists.
tests/injection/random_injection_test.ts (2)
20-78
: LGTM! Clean formatting improvements.The reformatting of the test structure enhances readability while maintaining the original functionality.
86-103
: LGTM! Well-structured test cases.The test cases for random lists and variant types are well-organized and properly formatted.
Also applies to: 110-171
tests/metagen/typegraphs/sample/py/main.py (1)
144-169
: LGTM! Good test coverage for nested composites.The new query comprehensively tests nested composite structures with various selection patterns:
- Scalar-only selections
- Struct selections
- Nested struct selections
- List selections
All using consistent SelectionFlags(select_all=True) for thorough coverage.
src/typegate/src/runtimes/substantial/workflow_worker_manager.ts (1)
130-130
: LGTM! Required permission for Deno v2.1.2.The addition of
import: true
to worker permissions is necessary for the Deno v2.1.2 upgrade to enable module imports in the worker environment.✅ Verification successful
Dynamic imports confirmed in worker files
The
import: true
permission is indeed required as multiple worker files use dynamic imports withtoFileUrl
, specifically in:
src/typegate/src/runtimes/substantial/worker.ts
src/typegate/src/runtimes/deno/worker.ts
src/typegate/src/runtimes/deno/hooks/worker.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if any workers are importing modules that would require this permission # Search for import statements in worker-related files rg -l "import .+ from" . --type ts --glob "**/worker*.ts"Length of output: 279
Script:
#!/bin/bash # Search for dynamic imports in worker-related files rg "import\s*\(" --type ts --glob "**/worker*.ts" -C 2 # Also check for specific Deno import APIs that would require this permission rg "Deno\.import" --type ts --glob "**/worker*.ts" -C 2Length of output: 1409
tests/metagen/typegraphs/sample/rs/main.rs (2)
139-166
: Well-structured query implementation with comprehensive test coverage.The nested composite query implementation demonstrates good practices:
- Progressive complexity in selections
- Proper use of the spread operator
- Clear structure for nested field selections
201-206
: LGTM: Consistent output structure.The integration of
res6
into the JSON output maintains the established pattern and naming conventions.tests/runtimes/wasm_wire/rust/fdk.rs (1)
112-112
: Version bump from rc.8 to rc.9.Clean version update that maintains compatibility checking in the
init
method.tests/runtimes/deno/deno_sync_test.ts (1)
279-281
: LGTM: Added sync prefix configuration.Appropriate addition of the sync prefix for the test engine configuration, which aligns with the test's purpose.
tests/e2e/published/published_test.ts (3)
82-86
: Improved download function implementation.The update to use
downloadFile
with structured parameters improves code clarity and maintainability.
371-379
: Enhanced error handling with better diagnostics.Good addition of error checking and detailed error reporting for typegate readiness.
346-346
: Added necessary Deno configuration.Added workspace configuration for the test environment.
tests/metagen/typegraphs/sample/rs/client.rs (1)
167-222
: Well-structured implementation of nested composite functionality.The implementation follows a clear hierarchical structure with proper separation of concerns:
- Root level composite structure
- Nested composite structure
- List structure handling
src/metagen/src/client_ts/static/mod.ts (1)
110-118
: Optimization: Skip processing of non-explicit composite selections.The added check improves performance by avoiding unnecessary processing when using selectAll flag. This change aligns with similar optimizations in the Python and Rust implementations.
src/metagen-client-rs/src/selection.rs (1)
Line range hint
396-404
: Consistent behavior change for composite selections.The modification to return
Self::Skip
instead of including all selections:
- Establishes a more conservative default behavior
- Reduces unnecessary processing
- Aligns with similar changes in TypeScript and Python implementations
tests/metagen/typegraphs/sample/ts_upload/client.ts (1)
113-118
: Consistent implementation of selectAll optimization.The implementation:
- Mirrors the changes in mod.ts
- Maintains consistency across the codebase
- Improves performance by skipping unnecessary processing
src/metagen/src/client_py/static/client.py (2)
107-110
: LGTM: Consistent variable namingThe change from
flags
tosub_flags
improves code clarity by making it explicit that these flags are for sub-selections.
123-132
: LGTM: Enhanced selection handlingThe new logic for skipping non-explicit composite selections when using
select_all
improves the control flow and prevents unnecessary node processing.tests/metagen/typegraphs/sample/py_upload/client.py (1)
126-134
: LGTM: Consistent implementationThe changes maintain consistency with the previous file, implementing the same selection handling improvements.
tests/metagen/typegraphs/sample/ts/client.ts (3)
977-1012
: LGTM: Well-structured node metadata implementationsThe new node metadata functions are well-organized and follow a consistent pattern, properly defining the structure for nested composites.
1043-1058
: LGTM: Clear type definitionsThe type definitions for nested composite structures are clear and properly typed.
1184-1191
: LGTM: Clean method implementationThe
nestedComposite
method implementation follows the established pattern and properly integrates with the existing codebase.import_map.json (3)
3-16
: LGTM: Updated to JSR registryThe migration to JSR (jsr:) for standard library imports aligns with modern Deno practices.
17-19
: LGTM: AWS SDK version updatesAWS SDK dependencies have been updated to version 3.723.0, maintaining security and compatibility.
45-66
: LGTM: Node.js compatibility layerThe addition of Node.js core module imports supports Deno's Node.js compatibility layer, which is essential for the Deno 2.1.2 update.
.ghjk/lock.json (2)
746-793
: Verify Rust toolchain configuration.The Rust toolchain has been updated with new configuration. Ensure all components and targets are necessary.
✅ Verification successful
Rust toolchain configuration is correctly configured ✓
All components and targets specified in the configuration are actively used in the codebase:
wasm32-unknown-unknown
target is used for WASM components in runtime testswasm32-wasi
target is used in metagen and examplesrust-src
component is required for IDE integration🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Rust components and targets usage # Check if all configured components and targets are actually used # Search for WASM target usage rg 'wasm32-(unknown-unknown|wasi)' # Search for rust-src component usage rg 'rust-src'Length of output: 1500
734-745
: Verify compatibility with new Deno versions.New Deno versions have been added. Ensure all Deno-dependent code is compatible with these versions.
✅ Verification successful
No compatibility issues detected with Deno version updates
The codebase uses only stable Deno APIs and no version-specific features. The version updates (v2.0.6 to v2.1.2) are minor and should maintain backward compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for Deno version-specific features in use # Look for usage of Deno APIs that might have changed between versions # Search for Deno API usage rg --type ts --type js 'Deno\.' -A 2Length of output: 40168
tests/metagen/typegraphs/sample/py/client.py (4)
126-134
: LGTM: Improved handling of select_all flag.The new logic correctly handles the select_all flag for non-explicit composite selections.
951-993
: LGTM: Well-structured nested composite type definitions.The new static methods for nested composite types are well-organized and follow the established pattern. The hierarchy is clear and maintainable.
Line range hint
999-1178
: LGTM: Well-defined type hierarchy.The TypedDict definitions are properly structured with correct typing and follow Python's type hinting best practices. The use of
total=False
makes the types more flexible.
1183-1190
: LGTM: Consistent implementation of new functionality.The type mapping and new method implementation are consistent with the existing codebase patterns and properly handle the new nested composite types.
Also applies to: 1288-1298
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.ghjk/lock.json
(16 hunks)tests/runtimes/deno/ts/math.ts
(1 hunks)tools/Dockerfile
(2 hunks)tools/Dockerfile.dockerignore
(1 hunks)tools/cross.Dockerfile
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tools/Dockerfile.dockerignore
- tools/Dockerfile
- tests/runtimes/deno/ts/math.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: pre-commit
🔇 Additional comments (7)
tools/cross.Dockerfile (2)
40-41
: LGTM! Good URL update.Updating the installation script URL from
raw.github.com
toraw.githubusercontent.com
is correct as it uses the official domain for raw content.
Line range hint
5-5
: Consider using a specific version tag instead of:main
Using the
:main
tag for the base image (ghcr.io/cross-rs/$CROSS_TARGET:main
) could lead to reproducibility issues as the image content may change unexpectedly. Consider using a specific version tag to ensure build reproducibility.Run this script to check available tags:
✅ Verification successful
The base image change is justified despite using the
:main
tagWhile using a specific version tag would typically be recommended for better reproducibility, moving away from an EOL Ubuntu 16 base image with outdated libraries is a significant improvement for security and functionality. The
ghcr.io/cross-rs/$CROSS_TARGET:main
image, even with the floating tag, is a better choice in this case.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check available tags for the cross-rs images # Note: This assumes the CROSS_TARGET is typically something like x86_64-unknown-linux-gnu # List available tags gh api \ -H "Accept: application/vnd.github+json" \ /orgs/cross-rs/packages/container/cross-rs%2Fx86_64-unknown-linux-gnu/versions \ --jq '.[].metadata.container.tags[]'Length of output: 376
Script:
#!/bin/bash # Search for any cross-rs related Dockerfile references rg -l "cross-rs" --type dockerfile # Search for any CROSS_TARGET usage in Dockerfiles rg "CROSS_TARGET" --type dockerfile -C 2 # List all Dockerfiles to analyze the context fd DockerfileLength of output: 231
Script:
#!/bin/bash echo "=== tools/cross.Dockerfile ===" cat tools/cross.Dockerfile echo -e "\n=== tools/Dockerfile ===" cat tools/DockerfileLength of output: 6020
.ghjk/lock.json (5)
740-745
: LGTM! Deno updated to v2.1.2.The update to Deno v2.1.2 aligns with the PR objectives.
Line range hint
854-1076
: LGTM! Environment sets consistently updated.All environment sets have been systematically updated to use the new version identifiers while maintaining the existing structure.
2143-2143
: LGTM! Default environment updated.The default environment identifier has been updated consistently.
Line range hint
2150-2161
: LGTM! Deno configuration properly structured.The blackboard configuration for Deno:
- Correctly specifies version 2.1.2
- Supports multiple platforms (Linux, Darwin, Windows)
- Maintains proper port configuration
746-794
: LGTM! Rust updated to 1.82.0 with enhanced WebAssembly support.The Rust update includes:
- Version bump to 1.82.0
- Profile set to "default"
- Added "rust-src" component
- Added WebAssembly targets
Run the following script to verify the Rust configuration:
✅ Verification successful
Verified: Rust 1.82.0 configuration is consistent with proper WebAssembly support
- All Rust instances are correctly updated to version 1.82.0
- WebAssembly targets (wasm32-unknown-unknown, wasm32-wasi) are properly configured
- The rust-src component is present where required
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Rust toolchain configuration # Check if rust-src component is available rustup component list | grep "rust-src" # Verify WebAssembly targets are installed rustup target list | grep -E "wasm32-(unknown-unknown|wasi)"Length of output: 296
Script:
#!/bin/bash # Search for all Rust configurations in lock.json echo "=== Rust Configurations ===" rg -A 20 '"version": "1\.82\.0"' .ghjk/lock.json echo -e "\n=== Version Consistency Check ===" # Extract and compare all Rust versions rg '"version":.+"1\.82\.0"' .ghjk/lock.json echo -e "\n=== WebAssembly Configurations ===" # Find all WebAssembly target configurations rg -B 5 -A 5 '"wasm32-' .ghjk/lock.jsonLength of output: 5116
<!-- Pull requests are squashed and merged using: - their title as the commit message - their description as the commit body Having a good title and description is important for the users to get readable changelog. --> <!-- 1. Explain WHAT the change is about --> - Closes [MET-786](https://linear.app/metatypedev/issue/MET-786/typescript-client-selectall-infinite-recursion). <!-- 3. Explain HOW users should update their code --> --- - [x] The change comes with new or modified tests - [ ] Hard-to-understand functions have explanatory comments - [ ] End-user documentation is updated to reflect the change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **New Features** - Added nested composite structure support across multiple client implementations - Enhanced selection handling for composite queries - Expanded type definitions for more complex data representations - **Bug Fixes** - Improved selection processing logic in client implementations - Updated version compatibility for SDK imports - **Chores** - Updated package dependencies to newer SDK versions - Reformatted and improved code readability across multiple files <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Update protobuf-codegen and protobuff to latest (3.7.1) --- - [ ] The change comes with new or modified tests - [ ] Hard-to-understand functions have explanatory comments - [ ] End-user documentation is updated to reflect the change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **Dependency Updates** - Updated `protobuf` library from version 3.5.1 to 3.7.1 - Updated `protobuf-codegen` crate to version 3.7.1 - **Build Configuration** - Modified protobuf compiler output option from `--rust_out` to `--rs_out` - Simplified protobuf code generation command - **Compatibility** - Ensured compatibility with latest protobuf runtime version <!-- end of auto-generated comment: release notes by coderabbit.ai -->
<!-- Pull requests are squashed and merged using: - their title as the commit message - their description as the commit body Having a good title and description is important for the users to get readable changelog. --> <!-- 1. Explain WHAT the change is about --> - Add a self-hosted json schema that will be referenced to [schemastore](https://www.schemastore.org/json/), part of [MET-798](https://linear.app/metatypedev/issue/MET-798/metatype-schema-for-ide-support). <!-- 3. Explain HOW users should update their code --> --- - [x] The change comes with new or modified tests - [ ] Hard-to-understand functions have explanatory comments - [ ] End-user documentation is updated to reflect the change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **New Features** - Added comprehensive configuration schema validation for Metatype system configuration files. - Implemented JSON schema testing to ensure configuration integrity across multiple YAML files. - **Tests** - Introduced new test suite for validating configuration schema using Ajv JSON schema validator. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/tests.yml (1)
289-290
:⚠️ Potential issueFix shell script issues in the test-full job.
The shell script issues are identical to those in the test-website job.
Apply the same fixes as suggested in the test-website job comment.
🧰 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)
🧹 Nitpick comments (1)
.github/workflows/tests.yml (1)
91-96
: Consider creating a reusable composite action.The installation steps are duplicated across multiple jobs. Consider creating a composite action to encapsulate these steps:
# .github/actions/install-ghjk/action.yml name: 'Install GHJK' description: 'Installs GHJK with proper environment setup' runs: using: "composite" steps: - shell: bash run: curl -fsSL "https://raw.githubusercontent.com/metatypedev/ghjk/$GHJK_VERSION/install.sh" | bash - shell: bash run: | echo "$HOME/.local/bin" >> "$GITHUB_PATH" echo "BASH_ENV=$HOME/.local/share/ghjk/env.sh" >> "$GITHUB_ENV" - shell: bash run: ghjk envs cook
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lock
is excluded by!**/*.lock
deno.lock
is excluded by!**/*.lock
📒 Files selected for processing (2)
.github/workflows/tests.yml
(5 hunks)tests/tools/schema_test.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/tools/schema_test.ts
🧰 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)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: bulid-docker (linux/arm64, custom-arm)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: test-website
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (2)
.github/workflows/tests.yml (2)
176-177
:⚠️ Potential issueFix shell script issues in the test-website job.
The current implementation has shell script issues that could cause problems:
- Tilde (
~
) doesn't expand in quotes- Missing quotes around variables could cause word splitting
Apply these fixes:
- - 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"Likely invalid or redundant comment.
🧰 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)
32-32
: Verify the impact of disabling GHJK logging.The removal of debug logging might impact troubleshooting capabilities in CI. Consider keeping it enabled or documenting alternative debugging methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tools/jsr/jsr-gen.ts (1)
Line range hint
31-73
: Consider these maintainability improvements.While the code works well, consider these enhancements:
- Extract the file extension regex pattern into a constant
- Add type definition for the configuration object
- Move magic strings in exclude arrays to constants
Here's how you could implement these improvements:
// At the top of the file const SUPPORTED_EXTENSIONS = /\.(ts|js|mjs)$/; const EXCLUDED_FILES = ["index.ts"] as const; const EXCLUDED_LINT_RULES = ["no-external-import"] as const; // Type for the configuration type DenoConfig = { name: string; version: string; publish: { exclude: string[]; }; lint: { rules: { exclude: string[]; }; }; exports: Record<string, string>; }; // Then use these in your code: if (SUPPORTED_EXTENSIONS.test(path)) { // ... rest of the code // And type the configuration: const config: DenoConfig = { name: SDK_PACKAGE_NAME_TS, version: CURRENT_VERSION, publish: { exclude: ["!src/gen", "!LICENSE.md", "!README.md"], }, lint: { rules: { exclude: EXCLUDED_LINT_RULES, }, }, exports: jsrExports, }; Deno.writeTextFileSync( join(denoSdkDir, "deno.json"), JSON.stringify(config, null, 2) + "\n" );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
deno.lock
is excluded by!**/*.lock
📒 Files selected for processing (5)
import_map.json
(1 hunks)src/typegate/src/transports/graphql/typegraph.ts
(0 hunks)src/typegate/src/typegraph/versions.ts
(1 hunks)tests/e2e/published/utils.ts
(0 hunks)tools/jsr/jsr-gen.ts
(1 hunks)
💤 Files with no reviewable changes (2)
- tests/e2e/published/utils.ts
- src/typegate/src/transports/graphql/typegraph.ts
✅ Files skipped from review due to trivial changes (1)
- src/typegate/src/typegraph/versions.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- import_map.json
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/arm64, custom-arm)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: test-full
- GitHub Check: test-website
- GitHub Check: pre-commit
🔇 Additional comments (1)
tools/jsr/jsr-gen.ts (1)
5-5
: LGTM! Good improvement in file existence checking.The removal of
existsSync
in favor of$.path(path).exists()
is a good change that aligns with Deno's idiomatic patterns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/tests.yml (2)
288-289
:⚠️ Potential issueUse $HOME instead of tilde in GitHub Actions.
GitHub Actions may not expand the tilde character correctly in all environments. Use $HOME instead for consistent behavior.
- - 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"
175-176
:⚠️ Potential issueUse $HOME instead of tilde in GitHub Actions.
GitHub Actions may not expand the tilde character correctly in all environments. Use $HOME instead for consistent behavior.
- - 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"
🧹 Nitpick comments (2)
src/typegate/engine/Cargo.toml (1)
58-58
: Add workspace dependency fordeno_error
This change integrates
deno_error
into the workspace, aligning with the overall dependency management improvements to support the updated Deno runtime. Please verify that this addition is compatible with Deno v2.1.2 and that any downstream modules using error handling from this crate have been updated accordingly..github/workflows/release.yml (1)
37-37
: Remove trailing whitespace in YAML file.There are trailing spaces on these lines that should be removed.
- +Also applies to: 185-185, 324-324
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 37-37: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lock
is excluded by!**/*.lock
deno.lock
is excluded by!**/*.lock
📒 Files selected for processing (38)
.ghjk/lock.json
(10 hunks).github/workflows/release.yml
(5 hunks).github/workflows/tests.yml
(5 hunks)Cargo.toml
(3 hunks)deno.jsonc
(2 hunks)examples/deno.jsonc
(0 hunks)import_map.json
(1 hunks)rust-toolchain.toml
(1 hunks)src/metagen-client-rs/Cargo.toml
(2 hunks)src/metagen-client-rs/src/common.rs
(2 hunks)src/mt_deno/src/lib.rs
(9 hunks)src/typegate/engine/00_runtime.js
(1 hunks)src/typegate/engine/Cargo.toml
(1 hunks)src/typegate/engine/bindings.ts
(22 hunks)src/typegate/engine/runtime.d.ts
(1 hunks)src/typegate/engine/runtime.js
(1 hunks)src/typegate/engine/src/error.rs
(1 hunks)src/typegate/engine/src/ext.rs
(3 hunks)src/typegate/engine/src/lib.rs
(5 hunks)src/typegate/engine/src/py_validation.rs
(2 hunks)src/typegate/engine/src/runtimes/grpc.rs
(6 hunks)src/typegate/engine/src/runtimes/prisma.rs
(9 hunks)src/typegate/engine/src/runtimes/substantial.rs
(29 hunks)src/typegate/engine/src/runtimes/temporal.rs
(14 hunks)src/typegate/engine/src/runtimes/wasm.rs
(3 hunks)src/typegate/engine/src/runtimes/wasm/conversion.rs
(1 hunks)src/typegate/engine/src/runtimes/wit_wire.rs
(2 hunks)src/typegate/engine/src/typegraph.rs
(1 hunks)src/typegate/src/engine/planner/args.ts
(1 hunks)src/typegate/src/engine/planner/policies.ts
(1 hunks)src/typegate/src/runtimes/patterns/worker_manager/deno.ts
(1 hunks)src/typegate/src/runtimes/python/hooks/mod.ts
(2 hunks)src/typegate/src/runtimes/substantial.ts
(1 hunks)src/typegate/src/runtimes/substantial/agent.ts
(1 hunks)src/typegate/src/runtimes/wit_wire/mod.ts
(1 hunks)src/typegate/src/services/artifact_service.ts
(2 hunks)src/typegate/src/services/graphql_service.ts
(2 hunks)src/typegate/src/typegate/mod.ts
(1 hunks)
💤 Files with no reviewable changes (1)
- examples/deno.jsonc
✅ Files skipped from review due to trivial changes (1)
- src/typegate/src/runtimes/wit_wire/mod.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- src/typegate/src/engine/planner/policies.ts
- src/typegate/src/runtimes/python/hooks/mod.ts
- rust-toolchain.toml
- deno.jsonc
- src/typegate/src/engine/planner/args.ts
- src/metagen-client-rs/Cargo.toml
- src/typegate/src/services/artifact_service.ts
- src/typegate/src/typegate/mod.ts
- src/typegate/engine/src/lib.rs
- src/typegate/engine/src/ext.rs
- src/mt_deno/src/lib.rs
- Cargo.toml
- src/typegate/engine/bindings.ts
🧰 Additional context used
🧬 Code Definitions (8)
src/typegate/engine/00_runtime.js (2)
src/typegate/engine/runtime.d.ts (1) (1)
Meta
(4-4)src/typegate/engine/runtime.js (2) (2)
Meta
(10-10)Meta
(10-10)
src/typegate/engine/runtime.js (2)
src/typegate/engine/00_runtime.js (1) (1)
Meta
(31-90)src/typegate/engine/runtime.d.ts (1) (1)
Meta
(4-4)
src/typegate/engine/src/py_validation.rs (1)
src/typegate/engine/src/error.rs (1) (1)
map
(17-19)
src/typegate/engine/src/typegraph.rs (3)
src/typegate/engine/src/runtimes/wasm.rs (1) (1)
serde_json
(35-35)src/typegate/engine/src/runtimes/wasm/conversion.rs (1) (1)
serde_json
(212-212)src/typegate/engine/src/error.rs (1) (1)
map
(17-19)
src/typegate/engine/src/runtimes/grpc.rs (4)
src/typegate/engine/src/error.rs (1) (1)
map
(17-19)src/typegate/engine/src/runtimes/prisma.rs (7) (7)
state
(59-59)state
(78-78)state
(109-109)state
(159-159)state
(190-190)state
(223-223)state
(242-242)src/typegate/engine/src/runtimes/substantial.rs (9) (9)
state
(63-63)state
(92-92)state
(125-125)state
(160-160)state
(187-187)state
(216-216)state
(244-244)state
(272-272)state
(293-293)src/typegate/engine/src/runtimes/wit_wire.rs (1) (1)
ctx
(168-170)
src/typegate/engine/src/runtimes/prisma.rs (3)
src/typegate/engine/src/error.rs (1) (1)
map
(17-19)src/typegate/engine/src/runtimes/prisma/engine.rs (1) (1)
query
(33-51)src/typegate/engine/src/runtimes/prisma/migration.rs (4) (4)
diff
(234-278)new
(80-87)new
(307-342)new
(352-361)
src/typegate/engine/src/runtimes/substantial.rs (4)
src/typegate/engine/runtime.d.ts (3) (3)
CreateOrGetOutput
(335-337)Run
(325-328)Operation
(323-323)src/typegate/engine/src/error.rs (1) (1)
map
(17-19)src/typegate/engine/src/runtimes/prisma.rs (7) (7)
state
(59-59)state
(78-78)state
(109-109)state
(159-159)state
(190-190)state
(223-223)state
(242-242)src/typegate/engine/src/runtimes/temporal.rs (5) (5)
state
(50-50)state
(90-90)state
(146-146)state
(191-191)state
(251-251)
src/typegate/src/services/graphql_service.ts (2)
src/typegate/src/services/auth/routes/validate.ts (1) (1)
badRequest
(7-9)src/typegate/src/services/responses.ts (2) (2)
badRequest
(57-61)jsonError
(32-55)
🪛 YAMLlint (1.35.1)
.github/workflows/tests.yml
[error] 172-172: trailing spaces
(trailing-spaces)
[error] 283-283: 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)
🪛 Biome (1.9.4)
src/typegate/src/services/graphql_service.ts
[error] 133-133: expected ,
but instead found :
Remove :
(parse)
[error] 133-133: expected ,
but instead found (
Remove (
(parse)
[error] 134-134: expected ,
but instead found :
Remove :
(parse)
[error] 135-135: expected ,
but instead found headers
Remove headers
(parse)
[error] 136-136: Expected an expression but instead found '}'.
Expected an expression here.
(parse)
[error] 136-136: Expected a statement but instead found ')'.
Expected a statement here.
(parse)
[error] 137-137: Expected a statement but instead found 'else'.
Expected a statement here.
(parse)
🔇 Additional comments (76)
src/typegate/src/services/graphql_service.ts (2)
55-59
: Error variable renaming for consistencyThe change from
e
toerr
for the caught error variable improves code consistency and readability. This convention is better aligned with standard practices for error handling.
123-145
: Error variable renaming for consistencyThe consistent renaming of error variable from
e
toerr
throughout this catch block aligns with the changes made earlier in the file, improving overall code consistency.🧰 Tools
🪛 Biome (1.9.4)
[error] 133-133: expected
,
but instead found:
Remove :
(parse)
[error] 133-133: expected
,
but instead found(
Remove (
(parse)
[error] 134-134: expected
,
but instead found:
Remove :
(parse)
[error] 135-135: expected
,
but instead foundheaders
Remove headers
(parse)
[error] 136-136: Expected an expression but instead found '}'.
Expected an expression here.
(parse)
[error] 136-136: Expected a statement but instead found ')'.
Expected a statement here.
(parse)
[error] 137-137: Expected a statement but instead found 'else'.
Expected a statement here.
(parse)
[error] 137-145: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.(lint/complexity/noUselessLoneBlockStatements)
src/typegate/src/runtimes/patterns/worker_manager/deno.ts (1)
37-37
: Enhanced worker permissions by enabling importsThis change enables the
import
permission for Deno workers, allowing them to dynamically import modules. This is likely a necessary update to maintain compatibility with Deno v2.1.2, which might have stricter default permission settings.The permission is appropriately placed in the non-overridable permissions section, maintaining the security boundary between typegraphs while enabling required functionality.
src/metagen-client-rs/src/common.rs (2)
243-245
: Improved memory efficiency with heap allocation for error responsesThe error handling has been updated to box the GraphQLResponse object when creating a RequestFailed error. This change helps reduce the memory footprint of the GraphQLRequestError enum variants.
292-292
: Optimized error enum memory layoutThe
RequestFailed
variant now stores the response as a boxed value (Box<GraphQLResponse>
) rather than inline, which optimizes the enum's memory layout. This is a good practice in Rust when dealing with variants that contain larger structs..ghjk/lock.json (6)
728-733
: Remove unnecessary intermediate Deno version.The v2.0.6 entry appears to be an unnecessary intermediate version since we're updating directly to v2.1.2. Having both versions could lead to confusion.
734-739
: Good addition of Deno v2.1.2.This implements the primary objective of the PR to update Deno to v2.1.2.
740-760
: Rust update looks good.Updating Rust to v1.82.0 aligns with the PR objectives. The configuration is properly structured.
761-788
: Enhanced Rust configuration with important components and targets.The addition of a Rust configuration with the "default" profile and essential components like "rust-src" and WebAssembly targets will improve development capabilities.
789-812
: Consider if all these Deno versions are necessary.The PR adds multiple additional Deno versions (v2.1.4, v2.1.5, v2.1.6, v2.1.10) beyond the target v2.1.2. Consider if all these intermediate versions are needed or if they might cause version management confusion.
2338-2381
: Appropriate Rust configuration in blackboard section.The updated Rust configuration properly reflects the changes to the version, profile, and additional components and targets.
src/typegate/engine/00_runtime.js (1)
92-92
:❓ Verification inconclusive
Global variable renamed to provide better isolation.
Changed from
globalThis.Meta
toglobalThis.____Meta
to create a more isolated namespace for the Meta object. This change works in conjunction with the explicit export of the Meta constant inruntime.js
, improving the module structure.Ensure this change is propagated correctly to all files that might have been directly accessing
globalThis.Meta
.
🏁 Script executed:
#!/bin/bash # Search for direct references to globalThis.Meta that might need to be updated rg "globalThis\.Meta" --type js --type tsLength of output: 43
Ensure all global references to Meta are updated for better namespace isolation
The change renaming from
globalThis.Meta
toglobalThis.____Meta
insrc/typegate/engine/00_runtime.js
(line 92) is intended to improve isolation and align with the exportedMeta
constant. The repository search forglobalThis.Meta
produced no matches, suggesting there are no direct static references remaining. However, the lack of results may be due to limitations in the search or indirect (dynamic) usages. Please perform a manual verification to ensure that no file—especially those employing dynamic property access—is still referencingglobalThis.Meta
.
- File to check:
src/typegate/engine/00_runtime.js
(line 92)- Diff snippet:
globalThis.____Meta = Meta;- Next step: Manually verify the entire codebase for any unintended reliance on
globalThis.Meta
, particularly in cases where dynamic property access might bypass static search tools.src/typegate/engine/runtime.d.ts (1)
4-4
: Export declaration of Meta improves modular usage.The change from a global declaration to an exported constant improves modularity by making
Meta
explicitly importable, which is a better practice for maintainable code. This approach provides clearer boundaries for the Meta interface.src/typegate/engine/src/runtimes/wasm/conversion.rs (1)
4-8
: Updated imports to use interlude module.The change replaces the direct import of
Context
fromanyhow
with a more comprehensive import from the local interlude module. This is part of the broader effort to standardize error handling with the newOpErr
type across the codebase.src/typegate/engine/runtime.js (1)
6-10
: Added proper export with type annotations for Meta.The addition of JSDoc type annotation and explicit export for
Meta
improves type safety and clarity. This aligns with the TypeScript declaration in runtime.d.ts, making it easier for consumers to understand the interface.src/typegate/src/runtimes/substantial.ts (1)
14-14
: Updated imports to include Meta for workflow operations.The import now explicitly marks
Backend
as a type and addsMeta
, which is needed for the workflow operations in#stopResolver
and#linkerResolver
methods. This improves type safety while providing access to necessary functionality..github/workflows/release.yml (4)
61-61
: LGTM! Updated prerelease condition to include 'rc' tags.The condition has been updated to include 'rc' tagged versions as prereleases alongside 'dev' versions, which is a good improvement for versioning practices.
106-109
: Verify the installation script URL domain across all instances.Same issue with using
raw.github.com
instead ofraw.githubusercontent.com
.- - run: curl -fsSL "https://raw.github.com/metatypedev/ghjk/$GHJK_VERSION/install.sh" | bash + - run: curl -fsSL "https://raw.githubusercontent.com/metatypedev/ghjk/$GHJK_VERSION/install.sh" | bash
186-190
: Verify the installation script URL domain across all instances.Same issue with using
raw.github.com
instead ofraw.githubusercontent.com
.- - run: curl -fsSL "https://raw.github.com/metatypedev/ghjk/$GHJK_VERSION/install.sh" | bash + - run: curl -fsSL "https://raw.githubusercontent.com/metatypedev/ghjk/$GHJK_VERSION/install.sh" | bash
326-329
: Verify the installation script URL domain across all instances.Same issue with using
raw.github.com
instead ofraw.githubusercontent.com
.- - run: curl -fsSL "https://raw.github.com/metatypedev/ghjk/$GHJK_VERSION/install.sh" | bash + - run: curl -fsSL "https://raw.githubusercontent.com/metatypedev/ghjk/$GHJK_VERSION/install.sh" | bash.github/workflows/tests.yml (4)
31-31
: LGTM! Commented out the GHJK_LOG debug variable.Appropriate change to disable debug logging in the CI environment.
92-95
: Verify the installation script URL domain for better security.The installation command is correctly quoted, but it's using
raw.github.com
which should be changed toraw.githubusercontent.com
for more reliable access.- - run: curl -fsSL "https://raw.github.com/metatypedev/ghjk/$GHJK_VERSION/install.sh" | bash + - run: curl -fsSL "https://raw.githubusercontent.com/metatypedev/ghjk/$GHJK_VERSION/install.sh" | bash
295-295
: Good addition ofset -x
for better debugging.Adding
set -x
to the shell script is beneficial for debugging as it will display each command as it's executed.
172-172
: Remove trailing whitespace in YAML file.There are trailing spaces on these lines that should be removed.
- +Also applies to: 283-283
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 172-172: trailing spaces
(trailing-spaces)
src/typegate/src/runtimes/substantial/agent.ts (1)
4-11
: LGTM! Improved TypeScript import organization.Good refactoring to explicitly separate type imports from value imports, which aligns with TypeScript best practices and creates clearer boundaries between types and runtime values.
src/typegate/engine/src/py_validation.rs (2)
4-4
: LGTM! Added the interlude module import.Adding the interlude module import provides access to common utilities and types, which aligns with the codebase's conventions.
22-22
: Improved error handling with OpErr type.The function now returns
Result<(), OpErr>
instead ofResult<()>
and usesmap_err(OpErr::map())
for explicit error handling. This is part of a systematic error handling refactoring across the codebase, improving consistency and error reporting.Also applies to: 24-24
src/typegate/engine/src/runtimes/wasm.rs (6)
25-25
: Updated return type for explicit error handling using OpErr.The function signature now explicitly specifies
OpErr
as the return error type, which aligns with the systematic approach to error handling across the codebase.
29-29
: Consistent error transformation using anyhow.Error handling pattern converted to use the
.into()
conversion toOpErr
, maintaining consistency with other error handling in the codebase.
42-42
: Improved error handling for file operations.Using
map_err(OpErr::map())
provides a cleaner and more consistent approach to error handling for file operations compared to the previous implementation.
46-48
: Standardized error handling for linker instantiation.The linker instantiation error handling now uses the
OpErr::map()
pattern, ensuring consistent error types throughout the function.
84-85
: Consistent error transformation for value conversion and serialization.Both the value conversion and JSON serialization now use the same error handling pattern with
OpErr::map()
, improving consistency and error reporting.
87-87
: Updated error handling for function call failures.Error handling for function calls now returns an
OpErr
using.into()
, completing the consistent error handling approach throughout the function.src/typegate/engine/src/typegraph.rs (4)
11-11
: Updated return type for explicit error handling.The function signature now explicitly specifies
OpErr
as the return error type, aligning with the codebase-wide error handling improvements.
12-13
: Improved error handling for JSON operations.Both deserialization and serialization now use the
OpErr::map()
pattern, ensuring consistent error types and better error reporting.
20-20
: Updated return type for validation function.The function now explicitly returns
Result<(), OpErr>
instead ofResult<()>
, clarifying the error type and aligning with the error handling pattern used throughout the codebase.
21-21
: Standardized error handling for deserialization.The deserialization error handling now uses
map_err(OpErr::map())
, ensuring consistent error types and improving error clarity.src/typegate/engine/src/runtimes/wit_wire.rs (4)
342-342
: Enhanced error handling with Deno JS error integration.The
WitWireInitError
enum now derivesdeno_error::JsError
, which allows for better integration with Deno's error handling system, consistent with the Deno v2.1.2 update.
344-356
: Expanded error variants with class annotations.The enum now includes:
- New
#[class(generic)]
attributes for JS error classification- Additional error variants for better error reporting:
UnexpectedMat
for unexpected material informationOther
for generic errorsModuleErr
for WebAssembly module errorsThis provides more specific error reporting, making debugging easier.
443-443
: Enhanced error handling for WitWireHandleError.Similar to
WitWireInitError
, this enum now derivesdeno_error::JsError
for better integration with Deno's error handling system.
445-450
: Added specific error variants for handle operations.The enum now includes:
#[class(generic)]
attributes for JS error classification- New error variants:
InstanceNotFound
with a descriptive id fieldModuleErr
for WebAssembly module errorsThis improves error specificity and reporting capabilities.
src/typegate/engine/src/runtimes/grpc.rs (5)
150-151
: Updated return type and error handling for gRPC registration.The function now:
- Returns
Result<(), OpErr>
instead ofResult<()>
, explicitly specifying the error type- Uses
map_err(OpErr::map())
for consistent error transformationThis aligns with the codebase-wide error handling improvements.
170-170
: Updated return type for gRPC unregistration.The function's return type changed to
Result<(), OpErr>
, explicitly specifying the error type and improving consistency.
173-174
: Enhanced error handling for client removal failures.The error handling now:
- Retains the descriptive context message
- Uses
map_err(OpErr::map())
to transform the error into the standardOpErr
typeThis ensures consistent error reporting throughout the codebase.
192-192
: Updated return type for gRPC method calls.The function now returns
Result<String, OpErr>
instead ofResult<String>
, explicitly specifying the error type and aligning with the error handling pattern used throughout the codebase.
201-229
: Standardized error handling throughout the gRPC call flow.Multiple error handling points have been updated to use
map_err(OpErr::map())
:
- Client retrieval (line 202)
- File descriptor operations (line 205)
- Method descriptor retrieval (line 208)
- Request creation (line 213)
- Path query parsing (line 215)
- Client readiness check (line 217)
- Unary call execution (line 229)
This ensures consistent error types and reporting throughout the entire operation flow.
src/typegate/engine/src/error.rs (5)
6-12
: Well-structured error type implementation.The
OpErr
struct is cleanly implemented as a wrapper aroundanyhow::Error
with a properFrom
trait implementation, allowing for seamless conversion fromanyhow::Error
.
13-20
: Good utility methods for error handling.The implementation provides useful utility methods like
get_error_class
andmap()
to facilitate error handling across the codebase. Themap()
function is particularly useful for easily converting various error types into theOpErr
type.
21-30
: Standard error trait implementations.Properly implementing
std::error::Error
andstd::fmt::Display
follows Rust best practices for error types, ensuring compatibility with the standard error handling mechanisms.
31-49
: Good Deno integration with JsErrorClass.The implementation of
deno_error::JsErrorClass
allows for seamless error propagation between Rust and JavaScript contexts, which is essential for the Deno runtime integration.
50-56
: Convenient Deref implementation.The
Deref
implementation allows direct access to the underlyinganyhow::Error
, providing convenience while maintaining the semantic benefits of a distinct error type.src/typegate/engine/src/runtimes/temporal.rs (6)
35-37
: Improved error handling in op_temporal_register.The function now returns
Result<(), OpErr>
and properly usesmap_err(OpErr::map())
to convert errors from building client options and connecting to the client. This creates a more consistent error handling pattern.Also applies to: 42-47
56-67
: Updated error handling in op_temporal_unregister.The function now returns
Result<(), OpErr>
and the error case now returns anErr
variant with a properly formatted error message instead of usinganyhow::bail!
. This is more consistent with the new error handling approach.
87-87
: Consistent error handling in op_temporal_workflow_start.The signature has been updated to return
Result<String, OpErr>
and the error handling now usesmap_err(OpErr::map())
consistently. This aligns with the project-wide error handling improvements.Also applies to: 96-97, 112-113
143-143
: Standardized error handling in op_temporal_workflow_signal.The function now returns
Result<(), OpErr>
and employs the same error handling pattern usingmap_err(OpErr::map())
for consistency across the codebase.Also applies to: 152-153, 166-167
187-187
: Unified error handling in op_temporal_workflow_query.The function signature has been updated to return
Result<Vec<String>, OpErr>
and error handling has been standardized usingmap_err(OpErr::map())
. The error case at the end also properly converts toOpErr
using.into()
.Also applies to: 198-199, 213-214, 223-223
248-248
: Consistent error handling in op_temporal_workflow_describe.The function now returns
Result<TemporalWorkflowDescribeOutput, OpErr>
and maintains the same error handling pattern withmap_err(OpErr::map())
.Also applies to: 262-263
src/typegate/engine/src/runtimes/prisma.rs (10)
52-55
: Improved error handling in op_prisma_register_engine.The function now returns
Result<(), OpErr>
and properly usesmap_err(OpErr::map())
for error handling, aligning with the new error handling approach across the codebase.Also applies to: 65-65
75-75
: Consistent error handling in op_prisma_unregister_engine.The function signature has been updated to return
Result<(), OpErr>
and the disconnect operation now properly converts errors usingmap_err(OpErr::map())
.Also applies to: 85-85
106-106
: Standardized error handling in op_prisma_query.The function now returns
Result<String, OpErr>
and uses the consistent error handling pattern withmap_err(OpErr::map())
.Also applies to: 111-113
129-135
: Unified error handling in op_prisma_diff.The function signature has been updated to return
Result<Option<(String, Vec<ParsedDiff>)>, OpErr>
and the same error handling pattern is applied to both the datamodel formatting and diff operations.
153-157
: Consistent error handling in op_prisma_apply.The function now returns
Result<PrismaApplyResult, OpErr>
and consistently applies the error handling pattern to the datamodel formatting, build, and apply operations.Also applies to: 164-167
184-188
: Standardized error handling in op_prisma_deploy.The function signature has been updated to return
Result<PrismaDeployOut, OpErr>
and maintains consistent error handling withmap_err(OpErr::map())
throughout.Also applies to: 195-195, 198-198
217-221
: Unified error handling in op_prisma_create.The function now returns
Result<PrismaCreateResult, OpErr>
and maintains the same error handling pattern across all operations.Also applies to: 228-228, 231-231
239-239
: Consistent error handling in op_prisma_reset.The function signature has been updated to return
Result<bool, OpErr>
and applies the same error handling pattern to the build and reset operations.Also applies to: 246-246, 249-249
260-261
: Standardized error handling in op_unpack.The function now returns
Result<(), OpErr>
and uses the consistent error conversion withmap_err(OpErr::map())
.
266-267
: Standardized error handling in op_archive.The function signature has been updated to return
Result<Option<String>, OpErr>
and maintains the same error handling pattern.import_map.json (4)
3-24
: Updated to current JSR standard library modules.The import map has been updated to use JSR (Deno's package registry) for standard library modules, which is the recommended approach for Deno 2.x. These updates ensure compatibility with the new Deno version.
25-27
: Updated AWS SDK dependencies.The AWS SDK dependencies have been updated from version 3.700.0 to 3.749.0 and changed from ESM.sh to NPM imports, which should offer better stability and compatibility.
48-83
: Added SDK and tooling dependencies.New imports have been added for local tools, typegraph SDK components, and various runtime implementations. This organization improves modularity and allows for better dependency management.
84-94
: Added development and testing dependencies.Additional dependencies for development and testing purposes have been added, including JSON Schema tools, PostgreSQL client, and test utilities, which should improve the development and testing experience.
src/typegate/engine/src/runtimes/substantial.rs (5)
60-60
: Improved error type specification throughout function signaturesAll functions now consistently return
Result<T, OpErr>
instead of justResult<T>
, making the error type explicit. This enhances type safety and provides clearer error handling contracts.Also applies to: 89-89, 122-122, 157-157, 184-184, 213-213, 241-241, 269-269, 290-290, 312-312, 340-340, 375-375, 408-408, 436-436, 465-465, 493-493
70-70
: Consistent error mapping for backend initializationBackend initialization errors are now properly mapped to
OpErr
usingmap_err(OpErr::map())
, ensuring consistent error handling throughout the codebase.Also applies to: 99-99, 132-132, 167-167, 193-194, 222-223, 250-251, 278-279, 299-300, 321-322, 349-350, 384-385, 417-418, 445-446, 474-475, 502-503
73-73
: Consistent error mapping for backend operationsAll backend operations now properly map their errors to the
OpErr
type, ensuring a uniform error handling approach across different operations. This improves error reporting and makes debugging easier.Also applies to: 101-104, 134-141, 169-177, 196-198, 225-227, 253-255, 281-283, 302-304, 324-326, 352-359, 387-393, 420-422, 448-450, 477-479, 505-507
169-177
: Properly handled nested Result with Option in read_scheduleThe error handling in
op_sub_store_read_schedule
correctly handles the complex case where an operation might return anOption<T>
wrapped in aResult
. The use oftranspose()
elegantly handles this nested structure.
308-308
: Added #[serde] annotations for consistent serializationThe
#[serde]
annotations have been added to functions that were missing them, ensuring consistent serialization behavior across all operations.Also applies to: 371-371, 404-404, 461-461
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.ghjk/lock.json (1)
728-733
: 🛠️ Refactor suggestionRemove Unnecessary Intermediate Deno Version Entry
The entry for"bciqg5gzspxjw7zvrubuyyqskh2244x6mkoxfwugnrx3clyadlegvidi"
with version"v2.0.6"
is now obsolete since the PR objective is to update Deno to v2.1.2. Removing this entry will simplify the dependency tree and avoid potential confusion.Proposed diff:
- "bciqg5gzspxjw7zvrubuyyqskh2244x6mkoxfwugnrx3clyadlegvidi": { - "version": "v2.0.6", - "buildDepConfigs": {}, - "portRef": "[email protected]", - "specifiedVersion": true - },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lock
is excluded by!**/*.lock
📒 Files selected for processing (5)
.ghjk/lock.json
(10 hunks)Cargo.toml
(3 hunks)ghjk.ts
(1 hunks)src/typegate/src/runtimes/wit_wire/mod.ts
(1 hunks)tools/consts.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/typegate/src/runtimes/wit_wire/mod.ts
- ghjk.ts
- tools/consts.ts
- Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: test-website
- GitHub Check: test-full
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: pre-commit
🔇 Additional comments (6)
.ghjk/lock.json (6)
734-739
: Confirm Deno Update to v2.1.2
The new entry for"bciqe7g5m4v5jkg3ubqhogjjntsduyrwxcirqcp6tc3jmjr5af7ojq6a"
correctly sets the version to"v2.1.2"
, which meets the PR objective.
740-760
: Rust Toolchain Upgrade to 1.82.0
The update within the"rust_rustup"
configuration (entry"bciqduktp7qkb4fwcygoacnivxdnnau6uctz4vb5ovujunqwajoqh4jq"
) now sets the version to"1.82.0"
. This change is consistent with the PR objective for updating Rust.
761-788
: Enhanced Rust Configuration Details
The subsequent entry ("bciqanj4cddv3wefsg5arfpx4zt5b4q7lok2eghvw5umfub76kxjs4bi"
) upgrades Rust to"1.82.0"
and revises its configuration by switching the profile from"minimal"
to"default"
and adding new components (e.g.,"rust-src"
) and targets (e.g.,"wasm32-unknown-unknown"
,"wasm32-wasi"
). This change considerably improves the Rust installation setup. Please verify that these modifications are intended for enhancing the build environment.
789-794
: Inconsistent Deno Version: v2.1.10 Encountered
The entry"bciqerpqq5txgtwiuqy3hvbtjwffexiyue7c5k4itdbslq3h4pegkzlq"
is set to"v2.1.10"
, which deviates from the expected"v2.1.2"
. Please confirm whether this higher version is intentional (for instance, if it applies to a different use case) or if it should be standardized to v2.1.2.
796-800
: Verify Deno Version Variant: v2.1.6
An entry ("bciqelfljimuhjjtwwwgqry5ho5zmmon3uzur6qem27cmmaegsn5pn4i"
) appears with version"v2.1.6"
. This is yet another variant alongside v2.1.2 and v2.1.10. It is recommended to review whether these discrepancies are deliberate or if the versions should be consolidated.
801-806
: Deno Version Inconsistency: v2.1.4 Detected
The entry"bciqjoiy67soflr46oqoknxpoy2m2qfr5n7x5kc6ed5ptprrbscmvoxy"
sets the version to"v2.1.4"
. Given the PR objective to update to v2.1.2, please check if this deviating version is required or if it should be adjusted for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
Cargo.toml
(3 hunks)import_map.json
(1 hunks)rust-toolchain.toml
(1 hunks)src/metagen/src/client_ts/static/mod.ts
(1 hunks)src/metagen/src/fdk_rs/mod.rs
(1 hunks)src/typegate/src/runtimes/s3.ts
(3 hunks)src/typegate/src/sync/typegraph.ts
(2 hunks)src/utils/mod.ts
(1 hunks)tests/metagen/metagen_test.ts
(2 hunks)tests/runtimes/s3/s3_test.ts
(4 hunks)tools/Dockerfile
(2 hunks)tools/compose/compose.base.yml
(2 hunks)tools/compose/compose.subredis.yml
(1 hunks)tools/consts.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- tools/compose/compose.base.yml
- src/metagen/src/client_ts/static/mod.ts
- tools/Dockerfile
🚧 Files skipped from review as they are similar to previous changes (6)
- src/typegate/src/runtimes/s3.ts
- rust-toolchain.toml
- tools/consts.ts
- tests/metagen/metagen_test.ts
- import_map.json
- Cargo.toml
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/typegate/src/sync/typegraph.ts (1)
src/utils/mod.ts (1)
createS3ClientWithMD5
(8-58)
🔇 Additional comments (8)
src/metagen/src/fdk_rs/mod.rs (1)
436-436
: LGTM! Correct target architecture update for WASI Preview 1.The change from
wasm32-wasi
towasm32-wasip1
aligns with the Rust toolchain update and properly reflects the WASI Preview 1 target naming convention. This ensures compatibility with the updated Rust version.src/typegate/src/sync/typegraph.ts (2)
16-16
: LGTM: Import addition for S3 client utility.The import of
createS3ClientWithMD5
aligns with the codebase standardization effort to use a consistent S3 client configuration across all modules.
35-37
: LGTM: S3 client initialization standardization.The change from direct
S3Client
instantiation to usingcreateS3ClientWithMD5
utility function ensures consistent S3 client behavior across the codebase, particularly for MD5 checksum handling in DeleteObjects operations.tests/runtimes/s3/s3_test.ts (4)
11-11
: LGTM: Import standardization for test consistency.The import change ensures that tests use the same S3 client configuration as the production code, improving test reliability and consistency.
20-28
: LGTM: S3 client initialization in tests.Using
createS3ClientWithMD5
in tests ensures consistency with production code and validates the MD5 checksum functionality.
40-53
: LGTM: Enhanced error handling for bucket cleanup.The addition of try-catch blocks with logging around bucket cleanup operations improves test robustness and debugging capabilities.
72-77
: LGTM: Error handling for bucket initialization.The error handling wrapper around
initBucket()
provides better visibility into initialization failures during test execution.src/utils/mod.ts (1)
8-58
:✅ Verification successful
Verify the middleware step and placement.
The middleware is added at the
finalizeRequest
step, but given that it needs to modify request headers, ensure this is the correct step in the AWS SDK middleware stack for header modifications.
🏁 Script executed:
#!/bin/bash # Description: Search for AWS SDK middleware documentation and examples to verify correct step usage # Search for middleware step usage examples in the codebase rg -A 10 -B 5 "finalizeRequest|buildRequest|initialize" --type ts # Look for other middleware implementations for reference rg -A 15 "middlewareStack\.add" --type tsLength of output: 3619
🌐 Web query:
AWS SDK for JavaScript v3 middleware stack steps and recommended step for modifying HTTP request headers
💡 Result:
The AWS SDK for JavaScript v3 uses a modular middleware stack approach to handle HTTP requests and responses. The middleware stack operates by executing middleware functions in a series of steps during the request lifecycle.
Middleware Stack Steps in AWS SDK v3
The middleware stack includes several predefined steps that represent different stages in processing an HTTP request. The main steps are:
- initialize: Initial setup and input processing.
- serialize: Converts the input into an HTTP request.
- build: Builds the HTTP request.
- finalizeRequest: Last step before the request is sent, useful for modifying headers or other request aspects.
- deserialize: Processes the HTTP response and converts it into the output type.
Recommended Step for Modifying HTTP Request Headers
To modify HTTP request headers, the recommended step is
finalizeRequest
. This step happens after the HTTP request is fully constructed, allowing you to safely add or modify headers before the request is sent out.Example Usage
Here is an example middleware that modifies HTTP headers in the
finalizeRequest
step:const addCustomHeaderMiddleware = (next, context) => async (args) => { // Modify headers here args.request.headers["X-Custom-Header"] = "MyValue"; // Pass control to the next middleware in the stack return next(args); }; // Add middleware to the middleware stack client.middlewareStack.add(addCustomHeaderMiddleware, { step: "finalizeRequest", name: "addCustomHeaderMiddleware", priority: "high", // Optional priority to control order });Summary
- The AWS SDK v3 middleware stack steps are: initialize, serialize, build, finalizeRequest, deserialize.
- To modify HTTP request headers, use the finalizeRequest step.
- Add your custom middleware to this step to safely modify or add headers.
Let me know if you want me to provide a detailed example or help with integrating this into your client!
Middleware placement is correct. The AWS SDK v3 documentation confirms that
finalizeRequest
is the recommended step for modifying or adding HTTP headers before the request is sent. No changes required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
♻️ Duplicate comments (9)
.github/workflows/release.yml (4)
38-42
: 🛠️ Refactor suggestionDuplicate: Secure
ghjk
install incheck-bump
jobAs in prior reviews, update the domain to
raw.githubusercontent.com
and ensure proper quoting of the URL:- - run: curl -fsSL "https://raw.github.com/metatypedev/ghjk/$GHJK_VERSION/install.sh" | bash + - run: curl -fsSL "https://raw.githubusercontent.com/metatypedev/ghjk/${GHJK_VERSION}/install.sh" | bash
106-110
: 🛠️ Refactor suggestionDuplicate: Secure
ghjk
install inpub-meta-cli
jobApply the same domain and quoting fix here:
- - run: curl -fsSL "https://raw.github.com/metatypedev/ghjk/$GHJK_VERSION/install.sh" | bash + - run: curl -fsSL "https://raw.githubusercontent.com/metatypedev/ghjk/${GHJK_VERSION}/install.sh" | bash
185-190
: 🛠️ Refactor suggestionDuplicate: Secure
ghjk
install inpub-typegraph
jobConsistently update this block as well:
- - run: curl -fsSL "https://raw.github.com/metatypedev/ghjk/$GHJK_VERSION/install.sh" | bash + - run: curl -fsSL "https://raw.githubusercontent.com/metatypedev/ghjk/${GHJK_VERSION}/install.sh" | bash🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 185-185: trailing spaces
(trailing-spaces)
324-330
: 🛠️ Refactor suggestionDuplicate: Secure
ghjk
install inbump
jobAnd here too, switch the domain and quote correctly:
- - run: curl -fsSL "https://raw.github.com/metatypedev/ghjk/$GHJK_VERSION/install.sh" | bash + - run: curl -fsSL "https://raw.githubusercontent.com/metatypedev/ghjk/${GHJK_VERSION}/install.sh" | bash🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 324-324: trailing spaces
(trailing-spaces)
.github/workflows/tests.yml (4)
91-95
: 🛠️ Refactor suggestionDuplicate: Secure
ghjk
install inpre-commit
jobAs before, use
raw.githubusercontent.com
and quoted URL:- - run: curl -fsSL "https://raw.github.com/metatypedev/ghjk/$GHJK_VERSION/install.sh" | bash + - run: curl -fsSL "https://raw.githubusercontent.com/metatypedev/ghjk/${GHJK_VERSION}/install.sh" | bash
140-147
: 🛠️ Refactor suggestionDuplicate: Secure
ghjk
install inlint-compat
jobUpdate this block similarly:
- - run: curl -fsSL "https://raw.github.com/metatypedev/ghjk/$GHJK_VERSION/install.sh" | bash + - run: curl -fsSL "https://raw.githubusercontent.com/metatypedev/ghjk/${GHJK_VERSION}/install.sh" | bash
173-177
: 🛠️ Refactor suggestionDuplicate: Fix
ghjk
install and path expansion intest-website
jobSwitch the domain, quote the URL, and replace
~
with$HOME
:- - run: curl -fsSL "https://raw.github.com/metatypedev/ghjk/$GHJK_VERSION/install.sh" | bash + - run: curl -fsSL "https://raw.githubusercontent.com/metatypedev/ghjk/${GHJK_VERSION}/install.sh" | bash - - run: echo "~/.local/bin" >> $GITHUB_PATH + - run: echo "$HOME/.local/bin" >> $GITHUB_PATH - - run: echo "BASH_ENV=~/.local/share/ghjk/env.sh" >> $GITHUB_ENV + - run: echo "BASH_ENV=$HOME/.local/share/ghjk/env.sh" >> $GITHUB_ENV
283-291
: 🛠️ Refactor suggestionDuplicate: Fix
ghjk
install and path expansion intest-full
jobAnd apply the same corrections here:
- - run: curl -fsSL "https://raw.github.com/metatypedev/ghjk/$GHJK_VERSION/install.sh" | bash + - run: curl -fsSL "https://raw.githubusercontent.com/metatypedev/ghjk/${GHJK_VERSION}/install.sh" | bash - - run: echo "~/.local/bin" >> $GITHUB_PATH + - run: echo "$HOME/.local/bin" >> $GITHUB_PATH - - run: echo "BASH_ENV=~/.local/share/ghjk/env.sh" >> $GITHUB_ENV + - run: echo "BASH_ENV=$HOME/.local/share/ghjk/env.sh" >> $GITHUB_ENV🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 283-283: trailing spaces
(trailing-spaces)
.ghjk/lock.json (1)
728-733
: Remove redundant intermediate Deno version entry
The lock file still includes the"v2.0.6"
entry fordeno_ghrel
, which appears to be an unnecessary holdover given the bump to later versions.
🧹 Nitpick comments (3)
.ghjk/lock.json (1)
2172-2174
: Prefix blackboard Deno version with ‘v’ for consistency
The blackboard entry forbciqejuax…
lists"version": "2.2.4"
. All other Deno versions use thev
prefix (e.g.,"v2.1.2"
). Consider updating to"v2.2.4"
for uniformity.tests/runtimes/prisma/query_builder_test.ts (1)
47-49
: Address the FIXME comment about polymorphic batch stage.The comment indicates that the test includes an unintended polymorphic batch stage from the initial plan. This should be tracked and resolved to ensure the test reflects the intended behavior.
Would you like me to create an issue to track this technical debt?
src/typegate/src/runtimes/prisma/prisma.ts (1)
314-316
: Enhance error message for better debugging.The duplicate check is good, but the error message could be more descriptive.
if (new Set(stageOrder).size != stageOrder.length) { - throw new Error("duplicate top stage ids"); + throw new Error(`Duplicate stage IDs found in batch query order: ${stageOrder.join(", ")}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
.ghjk/deno.lock
is excluded by!**/*.lock
Cargo.lock
is excluded by!**/*.lock
deno.lock
is excluded by!**/*.lock
📒 Files selected for processing (35)
.ghjk/lock.json
(10 hunks).github/workflows/publish-website.yml
(1 hunks).github/workflows/release.yml
(5 hunks).github/workflows/tests.yml
(5 hunks)Cargo.toml
(3 hunks)ghjk.ts
(1 hunks)import_map.json
(1 hunks)src/meta-cli/src/deploy/actors/task/deploy/artifacts.rs
(1 hunks)src/meta-cli/src/typegraph/rpc/mod.rs
(1 hunks)src/metagen/src/client_ts/static/mod.ts
(1 hunks)src/metagen/src/fdk_rs/mod.rs
(1 hunks)src/mt_deno/src/lib.rs
(9 hunks)src/typegate/engine/src/runtimes/wit_wire.rs
(3 hunks)src/typegate/src/engine/planner/policies.ts
(1 hunks)src/typegate/src/runtimes/prisma/prisma.ts
(4 hunks)src/typegate/src/runtimes/substantial.ts
(1 hunks)src/typegate/src/runtimes/substantial/agent.ts
(1 hunks)src/typegate/src/runtimes/wit_wire/mod.ts
(1 hunks)src/typegate/src/services/auth/protocols/oauth2.ts
(0 hunks)src/typegraph/core/src/runtimes/prisma/type_utils.rs
(1 hunks)src/typegraph/schema/src/validator/injection.rs
(1 hunks)src/typegraph/schema/src/validator/mod.rs
(1 hunks)src/typegraph/schema/src/validator/types.rs
(5 hunks)src/typegraph/schema/src/visitor.rs
(1 hunks)src/typegraph/schema/src/visitor2.rs
(1 hunks)src/utils/mod.ts
(1 hunks)tests/metagen/metagen_test.ts
(2 hunks)tests/runtimes/prisma/prisma_test.ts
(1 hunks)tests/runtimes/prisma/query_builder_test.ts
(1 hunks)tools/Dockerfile
(2 hunks)tools/compose/compose.base.yml
(3 hunks)tools/consts.ts
(1 hunks)tools/cross.Dockerfile
(1 hunks)tools/deps.ts
(1 hunks)tools/list-duplicates.ts
(2 hunks)
💤 Files with no reviewable changes (1)
- src/typegate/src/services/auth/protocols/oauth2.ts
✅ Files skipped from review due to trivial changes (11)
- ghjk.ts
- src/typegraph/schema/src/validator/mod.rs
- src/typegraph/core/src/runtimes/prisma/type_utils.rs
- tools/list-duplicates.ts
- src/meta-cli/src/deploy/actors/task/deploy/artifacts.rs
- src/typegraph/schema/src/visitor.rs
- tools/cross.Dockerfile
- src/typegate/src/runtimes/substantial/agent.ts
- tools/Dockerfile
- tools/consts.ts
- src/typegraph/schema/src/validator/types.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- src/metagen/src/fdk_rs/mod.rs
- src/typegate/src/runtimes/wit_wire/mod.ts
- src/meta-cli/src/typegraph/rpc/mod.rs
- tools/compose/compose.base.yml
- src/typegate/src/runtimes/substantial.ts
- src/typegate/src/engine/planner/policies.ts
- tests/metagen/metagen_test.ts
- src/metagen/src/client_ts/static/mod.ts
- tools/deps.ts
- Cargo.toml
- src/typegate/engine/src/runtimes/wit_wire.rs
- src/mt_deno/src/lib.rs
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/publish-website.yml
[error] 32-32: 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)
.github/workflows/tests.yml
[error] 172-172: trailing spaces
(trailing-spaces)
[error] 283-283: trailing spaces
(trailing-spaces)
🪛 Biome (1.9.4)
src/utils/mod.ts
[error] 22-78: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: build-docker (linux/arm64, ubuntu-22.04-arm)
- GitHub Check: build-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (7)
src/typegraph/schema/src/visitor2.rs (1)
22-22
: LGTM! Excellent lifetime simplification.The change from explicit lifetime
'a
to anonymous lifetime'_
follows modern Rust best practices and improves code readability without affecting functionality.src/typegraph/schema/src/validator/injection.rs (1)
21-21
: LGTM! Consistent lifetime annotation improvement.This change aligns with the broader pattern of lifetime simplifications across the codebase and follows Rust best practices by using anonymous lifetime
'_
where the compiler can infer the appropriate lifetime..ghjk/lock.json (3)
734-739
: Approve addition of Deno v2.1.2
The new[email protected]
entry aligns with the PR’s stated objective. The format and structure are correct.
842-845
: Verify environment sets include the correct Deno portRef
TheghjkEnvProvInstSet___main
(and other sets) add the"bciqejuaxov2wky6no34n5s7n5bxmxytz3h4bqqnfxnfgjegynrz7n6i"
install. Confirm this ID points to the intended Deno version after consolidation.
2383-2392
: Ensurerust_rustup
defaultInst matches configResolutions
The blackboarddefaultInst
forrust_rustup
is now"version": "1.85.0"
with default profile and components, but the configResolutions entries target1.82.0
. Please confirm and align these two sections.tests/runtimes/prisma/prisma_test.ts (1)
200-216
: Well-structured test for batch query functionality!The test effectively validates that multiple mutations can be batched in a single request and that each returns a distinct UUID.
src/typegate/src/runtimes/prisma/prisma.ts (1)
189-193
: Good fix for batch result handling!The change correctly addresses the previous limitation by processing all results in the batch instead of just the first item.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/typegate/src/typegate/artifacts/shared.ts (1)
174-178
: Consider adding error handling for file operations.The file write operations could fail due to permissions or disk space issues. Consider wrapping in try-catch for better error handling.
- await Deno.mkdir(dirname(targetFile), { recursive: true }); - const file = (await Deno.open(targetFile, { write: true, create: true })) - .writable; - await response.body!.pipeTo(file); + try { + await Deno.mkdir(dirname(targetFile), { recursive: true }); + const file = (await Deno.open(targetFile, { write: true, create: true })) + .writable; + await response.body!.pipeTo(file); + } catch (error) { + throw new ArtifactError( + `Failed to save artifact to local cache: ${error.message}`, + import.meta, + ); + }src/typegate/src/runtimes/s3.ts (1)
199-230
: Consider using Promise.allSettled for better error handling in parallel uploads.The current implementation using
Promise.all
will reject immediately if any upload fails, potentially leaving some uploads incomplete. Consider usingPromise.allSettled
to allow all uploads to complete and then handle failures appropriately.- // Upload all files in parallel - await Promise.all(files.map(async (file, i) => { + // Upload all files in parallel + const results = await Promise.allSettled(files.map(async (file, i) => { const filePath = `${prefix}${file.name}`; // ... rest of upload logic ... })); + + // Check for failures + const failures = results + .map((result, i) => ({ result, file: files[i] })) + .filter(({ result }) => result.status === 'rejected'); + + if (failures.length > 0) { + const errorMessages = failures + .map(({ result, file }) => `${file.name}: ${(result as PromiseRejectedResult).reason}`) + .join(', '); + throw new Error(`Failed to upload ${failures.length} file(s): ${errorMessages}`); + }.ghjk/lock.json (1)
2350-2389
: Inconsistent WASM target namingIn the
rust_rustup
default instance, targets include"wasm32-wasip1"
, whereas the config resolutions use"wasm32-wasi"
. Standardize on the correct target name (wasm32-wasi
) to avoid ambiguity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
deno.lock
is excluded by!**/*.lock
📒 Files selected for processing (11)
.ghjk/lock.json
(10 hunks).vscode/settings.json
(2 hunks)examples/typegraphs/metagen/rs/fdk.rs
(1 hunks)src/typegate/src/runtimes/s3.ts
(5 hunks)src/typegate/src/typegate/artifacts/shared.ts
(3 hunks)src/typegraph/specs/codegen/rpc/typescript/client.ts
(1 hunks)tests/artifacts/artifacts_test.ts
(1 hunks)tests/metagen/typegraphs/identities/rs/fdk.rs
(1 hunks)tests/metagen/typegraphs/identities/ts/fdk.ts
(1 hunks)tests/runtimes/wasm_wire/rust/fdk.rs
(1 hunks)tools/installs.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- examples/typegraphs/metagen/rs/fdk.rs
- .vscode/settings.json
- tests/metagen/typegraphs/identities/rs/fdk.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/installs.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/typegate/src/typegate/artifacts/shared.ts (2)
src/typegate/src/typegate/artifacts/local.ts (1)
fetch
(89-100)src/typegate/src/typegate/artifacts/mod.ts (1)
ArtifactError
(21-25)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: test-full
- GitHub Check: build-docker (linux/arm64, ubuntu-22.04-arm)
- GitHub Check: test-website
- GitHub Check: pre-commit
🔇 Additional comments (7)
tests/artifacts/artifacts_test.ts (1)
218-221
: LGTM! Improved error handling with better type safety.The changes enhance error handling by:
- Using a more descriptive variable name (
err
instead ofe
)- Adding a type guard to safely access the error message regardless of the thrown value type
- Following TypeScript best practices for exception handling
This aligns well with the broader error handling refactoring pattern mentioned across the codebase.
tests/metagen/typegraphs/identities/ts/fdk.ts (1)
828-828
: LGTM! Good TypeScript practice improvement.Adding the
override
keyword makes the method overriding explicit and provides compile-time safety. This aligns with TypeScript best practices and improves code maintainability.src/typegate/src/runtimes/s3.ts (1)
160-189
: Upload implementation using presigned URLs looks good!The refactored upload method properly:
- Generates presigned URLs with appropriate expiry
- Sets correct Content-Type and Content-Length headers
- Includes comprehensive error handling with response body
- Maintains good logging for debugging
tests/runtimes/wasm_wire/rust/fdk.rs (1)
119-119
: LGTM! Clean version bump.The version constant update from
"0.5.1-rc.3"
to"0.5.1-rc.4"
aligns with the broader project version updates mentioned in the PR objectives.src/typegraph/specs/codegen/rpc/typescript/client.ts (2)
82-84
: LGTM! Proper Buffer to Uint8Array migration.The replacement of Node.js
Buffer
withUint8Array
is appropriate for Deno 2.x compatibility and maintains the same functionality.
86-99
: LGTM! Clean unification of reading logic.The unified
do...while
loop approach is cleaner than the previous separate handling for Deno and Node environments. The manualUint8Array
concatenation correctly replacesBuffer.concat
, and the termination condition on newline byte0x0a
is appropriate..ghjk/lock.json (1)
2515-2521
: Align alias mapping forrust_rustup
The alias group now points to the new
rust_rustup
instance ID (bciqpbny46lpewoqqgsmjcpnjqpaqn62dnf6eiowmpif7ap6gthelm6q
). Ensure all downstream references (scripts, policies) are updated to this key to prevent resolution errors.
Signed-off-by: Yohe-Am <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 10
♻️ Duplicate comments (3)
.github/workflows/publish-website.yml (1)
34-34
: Quote the URL and use githubusercontent.com for reliability.As mentioned in the previous review, the URL should be properly quoted for security and reliability.
- - run: curl -fsSL https://raw.githubusercontent.com/metatypedev/ghjk/$GHJK_VERSION/install.sh | bash + - run: curl -fsSL "https://raw.githubusercontent.com/metatypedev/ghjk/${GHJK_VERSION}/install.sh" | bash.github/workflows/tests.yml (2)
172-172
: Remove trailing whitespace.Static analysis detected trailing spaces on these lines.
Also applies to: 283-283
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 172-172: trailing spaces
(trailing-spaces)
175-176
: 🛠️ Refactor suggestionUse $HOME instead of tilde for reliable path expansion.
The tilde (~) expansion may not work reliably in GitHub Actions. Use $HOME for consistency with other jobs.
Apply this fix:
- - 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"Also applies to: 288-289
🧹 Nitpick comments (2)
.github/workflows/publish-website.yml (1)
32-32
: Remove trailing spaces.Static analysis detected trailing spaces on this line.
- +🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 32-32: trailing spaces
(trailing-spaces)
.github/workflows/release.yml (1)
37-37
: Remove trailing whitespace.Static analysis detected trailing spaces on these lines.
Apply this fix to remove trailing spaces:
- +Also applies to: 185-185, 324-324
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 37-37: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
deno.lock
is excluded by!**/*.lock
📒 Files selected for processing (34)
.ghjk/lock.json
(11 hunks).github/workflows/publish-website.yml
(1 hunks).github/workflows/release.yml
(5 hunks).github/workflows/tests.yml
(5 hunks).pre-commit-config.yaml
(1 hunks)src/typegate/src/runtimes/python.ts
(0 hunks)src/typegate/src/runtimes/wasm_wire.ts
(0 hunks)src/typegate/src/typegate/mod.ts
(1 hunks)src/typegraph/deno/src/io.ts
(2 hunks)src/typegraph/deno/src/metagen.ts
(2 hunks)src/typegraph/deno/src/params.ts
(1 hunks)src/typegraph/deno/src/providers/aws.ts
(1 hunks)src/typegraph/deno/src/providers/temporal.ts
(1 hunks)src/typegraph/deno/src/runtimes/graphql.ts
(1 hunks)src/typegraph/deno/src/runtimes/random.ts
(1 hunks)src/typegraph/deno/src/runtimes/wasm.ts
(1 hunks)src/typegraph/deno/src/tg_manage.ts
(1 hunks)src/typegraph/deno/src/typegraph.ts
(1 hunks)src/typegraph/deno/src/utils/func_utils.ts
(1 hunks)src/typegraph/deno/src/utils/injection_utils.ts
(2 hunks)src/typegraph/specs/codegen/rpc/typescript/client.ts
(4 hunks)src/utils/mod.ts
(1 hunks)tests/e2e/cli/dev_test.ts
(3 hunks)tests/e2e/self_deploy/self_deploy_bad_cred_test.ts
(0 hunks)tests/e2e/self_deploy/self_deploy_bad_cred_test.ts.disabled
(1 hunks)tests/e2e/self_deploy/self_deploy_test.ts
(0 hunks)tests/e2e/self_deploy/self_deploy_test.ts.disable
(1 hunks)tests/metagen/metagen_test.ts
(2 hunks)tests/multi_typegraph/multi_typegraph.ts
(0 hunks)tests/patterns/worker_manager/pooling_test.ts
(2 hunks)tests/runtimes/python/python_sync_test.ts
(1 hunks)tests/runtimes/s3/s3_test.ts
(2 hunks)tools/installs.ts
(1 hunks)tools/list-duplicates.ts
(2 hunks)
💤 Files with no reviewable changes (5)
- src/typegate/src/runtimes/python.ts
- tests/multi_typegraph/multi_typegraph.ts
- src/typegate/src/runtimes/wasm_wire.ts
- tests/e2e/self_deploy/self_deploy_bad_cred_test.ts
- tests/e2e/self_deploy/self_deploy_test.ts
✅ Files skipped from review due to trivial changes (15)
- src/typegraph/deno/src/utils/func_utils.ts
- tests/runtimes/python/python_sync_test.ts
- src/typegraph/deno/src/params.ts
- src/typegraph/deno/src/typegraph.ts
- src/typegraph/deno/src/runtimes/wasm.ts
- src/typegraph/deno/src/providers/aws.ts
- src/typegraph/deno/src/runtimes/random.ts
- src/typegraph/deno/src/providers/temporal.ts
- tests/patterns/worker_manager/pooling_test.ts
- src/typegraph/deno/src/utils/injection_utils.ts
- src/typegraph/deno/src/metagen.ts
- tests/runtimes/s3/s3_test.ts
- tools/list-duplicates.ts
- src/typegraph/deno/src/io.ts
- tests/e2e/cli/dev_test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- tools/installs.ts
- src/typegate/src/typegate/mod.ts
- tests/metagen/metagen_test.ts
- src/utils/mod.ts
- src/typegraph/specs/codegen/rpc/typescript/client.ts
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/publish-website.yml
[error] 32-32: 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)
.github/workflows/tests.yml
[error] 172-172: trailing spaces
(trailing-spaces)
[error] 283-283: trailing spaces
(trailing-spaces)
🔇 Additional comments (14)
src/typegraph/deno/src/runtimes/graphql.ts (1)
4-4
: Excellent optimization using type-only import.Converting the
Effect
import to a type-only import is a best practice since it's only used for type annotations. This improves build performance and reduces bundle size.src/typegraph/deno/src/tg_manage.ts (1)
27-27
: Good simplification removing unnecessary async.The method doesn't perform any asynchronous operations, so removing the
async
keyword is correct and improves the API by making it synchronous..pre-commit-config.yaml (2)
44-50
: Verify the impact of disabling file type filtering.Commenting out the
types
configuration fordeno-lint
means it will now run on all files instead of just JavaScript/TypeScript files. This could significantly slow down pre-commit hooks and may cause unnecessary processing.Please confirm this change is intentional and consider the performance impact. If broader coverage is needed, consider using more specific
files
patterns instead.
56-58
: Verify the impact of disabling file type filtering for eslint.Similar to the deno-lint hook, removing
types_or
filtering means eslint will run on all files in thedocs/metatype.dev
directory, not just TypeScript files.Consider whether this broader scope is necessary or if it might cause performance issues.
.github/workflows/publish-website.yml (1)
32-37
: Manual ghjk setup correctly replaces the action.The manual installation steps properly recreate the functionality of the removed
metatypedev/setup-ghjk
action with correct PATH and environment variable setup.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 32-32: trailing spaces
(trailing-spaces)
.github/workflows/release.yml (2)
38-42
: LGTM! Manual ghjk installation is properly implemented.The replacement of the setup-ghjk action with manual installation steps is correctly implemented with proper quoting and the correct GitHub domain.
Also applies to: 106-109, 186-190, 326-329
61-61
: Prerelease condition updated to use 'rc' instead of 'alpha'.The change from checking for 'alpha' to 'rc' in the prerelease condition appears intentional for the release workflow.
tests/e2e/self_deploy/self_deploy_test.ts.disable (1)
1-45
: Well-structured disabled test awaiting feature completion.The test implementation looks solid with proper setup, assertions, and cleanup. The FIXME comment appropriately indicates this is disabled pending MET-754 implementation.
tests/e2e/self_deploy/self_deploy_bad_cred_test.ts.disabled (1)
1-41
: Good negative test case for authentication failure.The test properly implements negative testing with invalid credentials and correctly verifies the expected 401 Unauthorized error. The use of
unreachable()
ensures the test fails if deployment unexpectedly succeeds..github/workflows/tests.yml (2)
91-95
: LGTM! Manual ghjk installation correctly implemented.The replacement of the setup-ghjk action with manual installation steps is properly implemented with correct quoting and domain.
Also applies to: 144-147
295-295
: Shell tracing enabled for debugging.The addition of
set -x
will help with debugging the test-full job execution..ghjk/lock.json (3)
819-823
: Confirm Deno v2.2.4 entry
This block correctly adds the target Deno version v2.2.4. Verify that the blackboardmoduleSpecifier
and allallowedBuildDeps
include the new port ID so builds succeed.
825-847
: Approve Rust 1.85.0 config resolution
This block updatesrust_rustup
to 1.85.0 with the default profile andrust-src
component. It aligns with the PR objectives and no further changes are needed here.
2385-2424
: Approve defaultInst Rust version bump
ThedefaultInst
for therust_rustup
port is now set to 1.85.0 with the correct profile. This aligns with the toolchain upgrade—no further action required here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (89)
.github/workflows/tests.yml
(6 hunks)deno.jsonc
(4 hunks)src/metagen/fixtures/client_ts/main.ts
(1 hunks)src/typegate/src/config/types.ts
(1 hunks)src/typegate/src/engine/planner/args.ts
(2 hunks)src/typegate/src/engine/planner/mod.ts
(1 hunks)src/typegate/src/runtimes/deno/deno.ts
(1 hunks)src/typegate/src/runtimes/deno/hooks/mod.ts
(4 hunks)src/typegate/src/runtimes/deno/types.ts
(1 hunks)src/typegate/src/runtimes/deno/worker.ts
(1 hunks)src/typegate/src/runtimes/deno/worker_manager.ts
(1 hunks)src/typegate/src/runtimes/grpc.ts
(1 hunks)src/typegate/src/runtimes/patterns/worker_manager/deno.ts
(2 hunks)src/typegate/src/runtimes/patterns/worker_manager/mod.ts
(1 hunks)src/typegate/src/runtimes/patterns/worker_manager/pooling.ts
(1 hunks)src/typegate/src/runtimes/patterns/worker_manager/wasm.ts
(1 hunks)src/typegate/src/runtimes/python.ts
(1 hunks)src/typegate/src/runtimes/python/hooks/mod.ts
(2 hunks)src/typegate/src/runtimes/random.ts
(1 hunks)src/typegate/src/runtimes/s3.ts
(5 hunks)src/typegate/src/runtimes/substantial.ts
(1 hunks)src/typegate/src/runtimes/substantial/agent.ts
(1 hunks)src/typegate/src/runtimes/substantial/deno_context.ts
(1 hunks)src/typegate/src/runtimes/substantial/filter_utils.ts
(1 hunks)src/typegate/src/runtimes/substantial/types.ts
(1 hunks)src/typegate/src/runtimes/substantial/worker.ts
(1 hunks)src/typegate/src/runtimes/substantial/workflow_worker_manager.ts
(1 hunks)src/typegate/src/runtimes/typegraph.ts
(1 hunks)src/typegate/src/runtimes/typegraph/helpers.ts
(1 hunks)src/typegate/src/runtimes/typegraph/type_emitter.ts
(1 hunks)src/typegate/src/runtimes/typegraph/visibility.ts
(1 hunks)src/typegate/src/runtimes/utils/deno.ts
(1 hunks)src/typegate/src/runtimes/wasm/worker.ts
(1 hunks)src/typegate/src/runtimes/wasm/worker_manager.ts
(1 hunks)src/typegate/src/runtimes/wasm_reflected.ts
(1 hunks)src/typegate/src/runtimes/wasm_wire.ts
(1 hunks)src/typegate/src/sync/typegraph.ts
(2 hunks)src/typegate/src/transports/graphql/typegraph.ts
(1 hunks)src/typegate/src/typegate/hooks.ts
(2 hunks)src/typegate/src/typegate/mod.ts
(2 hunks)src/typegate/src/typegate/rate_limiter.ts
(1 hunks)src/typegate/src/typegraph/mod.ts
(1 hunks)src/typegate/src/typegraph/utils.ts
(1 hunks)src/typegate/src/types.ts
(1 hunks)src/typegate/src/utils/mod.ts
(1 hunks)tests/artifacts/artifacts_test.ts
(2 hunks)tests/auth/auth_test.ts
(1 hunks)tests/dedup/dedup_test.ts
(1 hunks)tests/e2e/cli/dev_test.ts
(3 hunks)tests/e2e/cli/watch_test.ts
(1 hunks)tests/e2e/published/common.ts
(1 hunks)tests/e2e/typegraph/typegraph_test.ts
(1 hunks)tests/e2e/website/website_test.ts
(1 hunks)tests/metagen/metagen_test.ts
(3 hunks)tests/metagen/typegraphs/sample/ts/main.ts
(1 hunks)tests/policies/policies_test.ts
(1 hunks)tests/rate_limiter/rate_limiter_test.ts
(1 hunks)tests/runtimes/graphql/graphql_test.ts
(1 hunks)tests/runtimes/grpc/grpc_test.ts
(1 hunks)tests/runtimes/kv/kv_test.ts
(1 hunks)tests/runtimes/prisma/one_to_one_test.ts
(1 hunks)tests/runtimes/prisma/query_builder_test.ts
(1 hunks)tests/runtimes/prisma/schema_generation_test.ts
(1 hunks)tests/runtimes/prisma/type_duplication_test.ts
(1 hunks)tests/runtimes/python/python_sync_test.ts
(1 hunks)tests/runtimes/python/python_test.ts
(1 hunks)tests/runtimes/substantial/common.ts
(1 hunks)tests/runtimes/substantial/filter_utils_test.ts
(1 hunks)tests/runtimes/substantial/workflows/child_workflow.ts
(1 hunks)tests/runtimes/substantial/workflows/workflow.ts
(1 hunks)tests/runtimes/temporal/temporal_test.ts
(1 hunks)tests/runtimes/wasm_wire/wasm_wire_test.ts
(1 hunks)tests/sync/sync_config_test.ts
(1 hunks)tests/sync/sync_force_remove_test.ts
(2 hunks)tests/type_nodes/union_test.ts
(1 hunks)tests/utils/hooks.ts
(1 hunks)tests/utils/meta.ts
(1 hunks)tests/utils/migrations.ts
(1 hunks)tests/utils/mock_fetch.ts
(1 hunks)tests/utils/mod.ts
(1 hunks)tests/utils/query/file_extractor.ts
(1 hunks)tests/utils/query/graphql_query.ts
(1 hunks)tests/utils/query/mod.ts
(1 hunks)tests/utils/query/rest_query.ts
(1 hunks)tests/utils/single_register.ts
(1 hunks)tests/utils/test.ts
(2 hunks)tests/utils/test_module.ts
(1 hunks)tools/list-duplicates.ts
(2 hunks)tools/tree-view-web.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (52)
- tests/e2e/typegraph/typegraph_test.ts
- tests/runtimes/substantial/workflows/workflow.ts
- src/typegate/src/runtimes/wasm_reflected.ts
- tests/runtimes/wasm_wire/wasm_wire_test.ts
- tests/runtimes/substantial/workflows/child_workflow.ts
- tests/runtimes/grpc/grpc_test.ts
- src/typegate/src/runtimes/utils/deno.ts
- src/typegate/src/config/types.ts
- src/typegate/src/runtimes/substantial/deno_context.ts
- src/typegate/src/runtimes/wasm_wire.ts
- src/typegate/src/runtimes/deno/types.ts
- src/typegate/src/runtimes/substantial/types.ts
- tests/rate_limiter/rate_limiter_test.ts
- src/metagen/fixtures/client_ts/main.ts
- src/typegate/src/runtimes/patterns/worker_manager/wasm.ts
- src/typegate/src/typegraph/utils.ts
- tests/runtimes/prisma/query_builder_test.ts
- src/typegate/src/runtimes/wasm/worker.ts
- src/typegate/src/runtimes/deno/deno.ts
- tests/utils/query/mod.ts
- tests/runtimes/kv/kv_test.ts
- tests/auth/auth_test.ts
- src/typegate/src/types.ts
- tests/metagen/typegraphs/sample/ts/main.ts
- src/typegate/src/runtimes/substantial/workflow_worker_manager.ts
- tests/runtimes/prisma/type_duplication_test.ts
- tests/sync/sync_config_test.ts
- src/typegate/src/runtimes/python.ts
- tests/utils/query/rest_query.ts
- tests/utils/meta.ts
- tests/utils/migrations.ts
- src/typegate/src/runtimes/substantial/filter_utils.ts
- tests/runtimes/prisma/schema_generation_test.ts
- tests/e2e/cli/watch_test.ts
- tests/dedup/dedup_test.ts
- src/typegate/src/runtimes/wasm/worker_manager.ts
- tests/utils/mod.ts
- tests/runtimes/python/python_test.ts
- tests/runtimes/python/python_sync_test.ts
- src/typegate/src/typegate/rate_limiter.ts
- tests/e2e/published/common.ts
- src/typegate/src/runtimes/patterns/worker_manager/mod.ts
- src/typegate/src/runtimes/substantial/agent.ts
- src/typegate/src/runtimes/patterns/worker_manager/pooling.ts
- src/typegate/src/runtimes/typegraph.ts
- src/typegate/src/runtimes/typegraph/type_emitter.ts
- tests/utils/query/graphql_query.ts
- tests/policies/policies_test.ts
- src/typegate/src/runtimes/typegraph/visibility.ts
- tests/utils/single_register.ts
- src/typegate/src/transports/graphql/typegraph.ts
- tests/runtimes/prisma/one_to_one_test.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- src/typegate/src/runtimes/patterns/worker_manager/deno.ts
- src/typegate/src/runtimes/python/hooks/mod.ts
- tests/artifacts/artifacts_test.ts
- tools/list-duplicates.ts
- src/typegate/src/engine/planner/args.ts
- deno.jsonc
- tests/metagen/metagen_test.ts
- src/typegate/src/sync/typegraph.ts
- src/typegate/src/typegate/mod.ts
- tests/sync/sync_force_remove_test.ts
- src/typegate/src/typegate/hooks.ts
- src/typegate/src/runtimes/deno/hooks/mod.ts
- .github/workflows/tests.yml
- tests/utils/test.ts
- tests/e2e/cli/dev_test.ts
- src/typegate/src/runtimes/s3.ts
- src/typegate/src/runtimes/substantial.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test-full
- GitHub Check: build-docker (linux/arm64, ubuntu-22.04-arm)
- GitHub Check: build-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: test-website
- GitHub Check: pre-commit
🔇 Additional comments (23)
tests/type_nodes/union_test.ts (1)
4-4
: LGTM! Good use of type-only import.Converting to a type-only import is appropriate here since
JSONValue
is only used for type annotations throughout the file. This improves clarity and aligns with TypeScript best practices.tests/runtimes/temporal/temporal_test.ts (1)
6-6
: LGTM! Good use of type-only import.Converting to a type-only import is appropriate since
MetaTest
is only used for type annotations. This improves bundling efficiency and makes the code intent clearer.tests/runtimes/graphql/graphql_test.ts (1)
4-4
: LGTM! Good practice using type-only imports.The conversion to a type-only import is appropriate since
QueryEngine
is only used for type annotations (line 16) and not as a runtime value. This improves bundle efficiency and clarifies the intent of the import.tests/utils/mock_fetch.ts (1)
11-11
: LGTM! Clean TypeScript type-only import improvement.The use of
type
modifiers forMatchHandler
andRoutes
correctly identifies these as type-only imports, improving bundling efficiency and type clarity. Therouter
function is appropriately left without the modifier since it's used at runtime.src/typegate/src/runtimes/substantial/worker.ts (1)
7-7
: LGTM! Proper use of type-only imports.The conversion to type-only imports for
WorkflowEvent
andWorkflowMessage
is correct since these are only used as types throughout the file (in type annotations andsatisfies
clauses). This improves code clarity and bundling efficiency while maintainingInterrupt
as a runtime import since it's used forInterrupt.getTypeOf()
.src/typegate/src/engine/planner/mod.ts (3)
21-21
: Excellent use of type-only import for better code clarity.The
type
modifier forStageMetadata
correctly indicates this import is used solely for type annotations, improving code clarity and bundling efficiency.
26-26
: Good modernization with type-only import syntax.Converting to
import type
forRuntime
follows TypeScript best practices and clearly indicates this is used only for type annotations.
28-28
: Proper type-only import implementation.The
import type
syntax forInjection
correctly reflects its usage as a type annotation only, aligning with the codebase modernization effort.tests/runtimes/substantial/filter_utils_test.ts (1)
8-9
: Excellent use of type-only imports!Converting
ExecutionStatus
andExpr
to type-only imports is the correct approach since they're used exclusively for type annotations throughout the file. This improves clarity and can reduce bundle size.tests/runtimes/substantial/common.ts (1)
7-8
: Well-executed type-only import conversion!Converting
MetaTestCleanupFn
andExpr
to type-only imports is appropriate since they're used exclusively for type annotations (function parameters and type constraints). This change aligns with TypeScript best practices and the broader pattern mentioned in the PR.src/typegate/src/runtimes/typegraph/helpers.ts (1)
5-6
: LGTM! Proper use of type-only imports.The conversion to
import type
is appropriate sinceTypeGraphDS
,ObjectNode
, andPolicyIndices
are only used for type annotations in function parameters and return types.tools/tree-view-web.ts (1)
11-11
: LGTM! Correct type-only import usage.The conversion to
import type
is appropriate sinceTypeGraphDS
is only used for the array type annotation on line 42.src/typegate/src/typegraph/mod.ts (1)
42-42
: LGTM! Proper mixed import pattern.The change correctly uses
type
keyword forGeneratorNode
(used only for type annotations) while keepingrandomizeRecursively
as a runtime import (used as a function call on line 334).src/typegate/src/runtimes/random.ts (2)
5-5
: LGTM! Appropriate type-only import.The conversion to
import type
is correct sinceComputeStage
is only used for type annotations in function parameters and return types.
11-11
: LGTM! Appropriate type-only import.The conversion to
import type
is correct sinceTypeGraphDS
is only used for the class property type annotation on line 21.tests/utils/test_module.ts (1)
5-5
: Excellent use of type-only imports!This change correctly converts
ShellOptions
andShellOutput
to type-only imports while keepingshell
as a runtime import. This improves bundling efficiency and makes the code more explicit about compile-time vs runtime dependencies.tests/utils/query/file_extractor.ts (1)
4-4
: Proper type-only import conversion.The
Variables
import is correctly converted to type-only since it's used solely for the parameter type annotation inextractFilesFromVars
. This follows TypeScript best practices and improves bundling efficiency.tests/e2e/website/website_test.ts (2)
7-7
: Correct type-only import for MetaTest.The
MetaTest
import is properly converted to type-only since it's used exclusively for type annotations in function parameters and the prepare record type definition.
9-9
: Correct type-only import for TypeGraphDS.The
TypeGraphDS
import is appropriately converted to type-only since it's used only for type annotations in array type casting and function parameters. This improves bundling efficiency and code clarity.src/typegate/src/runtimes/grpc.ts (1)
7-7
: LGTM! Correct implementation of type-only imports.The conversion to
import type
forLogger
,Resolver
, andRuntimeInitParams
is appropriate since these are used solely for type annotations. This improves compilation efficiency and code clarity.Also applies to: 9-9
src/typegate/src/runtimes/deno/worker.ts (1)
5-5
: LGTM! Proper type-only import usage.Converting
DenoEvent
andDenoMessage
to type-only imports is correct since they're used solely for type annotations (MessageEvent<DenoMessage>
andsatisfies DenoEvent
).src/typegate/src/runtimes/deno/worker_manager.ts (1)
12-15
: LGTM! Excellent distinction between runtime and type-only imports.The import changes correctly categorize each import:
TaskId
,TaskContext
,DenoEvent
,DenoMessage
,TaskSpec
→ type-only (used solely for type annotations)hostcall
→ runtime import (used as function call)HostCallCtx
→ type-only (used for parameter typing)This mixed import pattern on line 13 is a great example of proper TypeScript import semantics.
tests/utils/hooks.ts (1)
4-4
: LGTM! Type-only imports correctly applied.The conversion to type-only imports for
SyncConfig
andS3ClientConfig
is appropriate since these are only used as type annotations.S3Client
correctly remains a regular import as it's used for runtime instantiation.Also applies to: 7-7
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #923 +/- ##
==========================================
- Coverage 80.99% 80.57% -0.43%
==========================================
Files 143 145 +2
Lines 18046 17341 -705
Branches 1967 1968 +1
==========================================
- Hits 14616 13972 -644
+ Misses 3412 3351 -61
Partials 18 18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores