feat: TypeScript support in charts#502
Conversation
internal/tschart/transformer.go
Outdated
|
|
||
| nodeModulesPath := filepath.Join(tsDir, "node_modules") | ||
| if _, err := os.Stat(nodeModulesPath); os.IsNotExist(err) { | ||
| log.Default.Debug(ctx, "No node_modules directory found, skipping vendor bundle") |
There was a problem hiding this comment.
We should handle these cases:
- node_modules are up-to-date: proceed
- node_modules are not-up-to-date: error
- node_modules are empty/nonexistent, but there are dependencies in the project: error
- node_modules are empty/nonexistent, but there are no dependencies in the project: proceed
- node_modules are not empty, but there are no dependencies: error
Not sure how can we do it. And can we do this reliably and programmatically at all.
|
@ilya-lesikov I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found across 23 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="internal/tschart/transformer_mem.go">
<violation number="1" location="internal/tschart/transformer_mem.go:217">
P1: Using `filepath.Join` in virtual filesystem context will cause cross-platform issues. On Windows, this produces backslash-separated paths that won't match the forward-slash keys in the files map. Use `path.Join` from the `path` package instead, which always uses forward slashes.</violation>
</file>
<file name="internal/tschart/init.go">
<violation number="1" location="internal/tschart/init.go:331">
P2: The `chartName` is directly interpolated into JSON without escaping. If the chart name contains special JSON characters (quotes, backslashes), this produces invalid JSON. Consider using `json.Marshal` for proper escaping or validate the chart name before use.</violation>
</file>
<file name="internal/tschart/runtime.go">
<violation number="1" location="internal/tschart/runtime.go:104">
P2: Calling `ToObject(nil)` with nil runtime is unsafe and could cause a panic. The `formatJSError` function needs access to the goja runtime to safely convert the error value to an object. Consider passing the `*goja.Runtime` as a parameter, or add defensive nil checks.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
internal/tschart/runtime.go
Outdated
|
|
||
| errMsg := gojaErr.Error() | ||
|
|
||
| stackProp := gojaErr.Value().ToObject(nil).Get("stack") |
There was a problem hiding this comment.
P2: Calling ToObject(nil) with nil runtime is unsafe and could cause a panic. The formatJSError function needs access to the goja runtime to safely convert the error value to an object. Consider passing the *goja.Runtime as a parameter, or add defensive nil checks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/tschart/runtime.go, line 104:
<comment>Calling `ToObject(nil)` with nil runtime is unsafe and could cause a panic. The `formatJSError` function needs access to the goja runtime to safely convert the error value to an object. Consider passing the `*goja.Runtime` as a parameter, or add defensive nil checks.</comment>
<file context>
@@ -0,0 +1,112 @@
+
+ errMsg := gojaErr.Error()
+
+ stackProp := gojaErr.Value().ToObject(nil).Get("stack")
+ if stackProp == nil || goja.IsUndefined(stackProp) || goja.IsNull(stackProp) {
+ return fmt.Errorf("%s\n at %s", errMsg, currentFile)
</file context>
|
/review |
There was a problem hiding this comment.
Code Review Agent Run #aee90b
Actionable Suggestions - 4
-
ts/nelm-types/src/index.ts - 1
- Type mismatch in Tags field · Line 28-28
-
internal/chart/chart_render.go - 1
- Bug: Wrong path for TS rendering · Line 212-212
-
internal/tschart/init.go - 1
- Incorrect build script · Line 337-337
-
cmd/nelm/chart_init.go - 1
- Input Validation Missing · Line 40-40
Additional Suggestions - 5
-
internal/tschart/runtime.go - 1
-
Invalid goja API usage · Line 104-104The ToObject(nil) call on line 104 violates goja's API, as Value.ToObject requires a valid *goja.Runtime. This could cause a panic during error formatting when JavaScript exceptions occur. If the runtime isn't available here, consider restructuring to pass it from the caller.
-
-
internal/tschart/transformer_unit_test.go - 2
-
Incomplete test assertions · Line 383-401The tests for generateVendorEntrypoint check the var declaration, require statements, and global assignment, but miss verifying the exports assignment. This could allow regressions if the exports line is removed from the function. Consider adding checks for 'exports.__NELM_VENDOR__ = __NELM_VENDOR__' in both test cases.
-
Test Isolation Improvement · Line 40-40The test for non-existent path uses a relative path not isolated in the test's tempDir, unlike other tests that use tempDir. This could potentially cause the test to fail if the current working directory has a file or directory with that name. Using filepath.Join(tempDir, "non-existent") would ensure proper isolation.
-
-
internal/tschart/init_test.go - 1
-
Missing Error Test Case · Line 155-257The test suite for InitChartStructure is missing coverage for the error case where the ts/ directory already exists. While the implementation correctly checks and returns an error in this scenario (matching InitTSBoilerplate's behavior), adding this test would ensure the error path is verified, preventing potential regressions if the check is accidentally removed.
-
-
pkg/action/chart_init.go - 1
-
Unused struct field · Line 17-17The TempDirPath field in ChartInitOptions is defined but not referenced in ChartInit or its callees. If this is for future temp dir logic, consider adding a comment; otherwise, remove to avoid confusion.
-
Review Details
-
Files reviewed - 21 · Commit Range:
9a6e649..37f16e7- cmd/nelm/chart.go
- cmd/nelm/chart_init.go
- cmd/nelm/chart_pack.go
- go.mod
- go.sum
- internal/chart/chart_render.go
- internal/tschart/console.go
- internal/tschart/engine.go
- internal/tschart/engine_unit_test.go
- internal/tschart/init.go
- internal/tschart/init_test.go
- internal/tschart/runtime.go
- internal/tschart/test_helpers_test.go
- internal/tschart/transformer.go
- internal/tschart/transformer_dir.go
- internal/tschart/transformer_mem.go
- internal/tschart/transformer_unit_test.go
- internal/tschart/tschart_test.go
- pkg/action/chart_init.go
- pkg/featgate/feat.go
- ts/nelm-types/src/index.ts
-
Files skipped - 2
- ts/nelm-types/package.json - Reason: Filter setting
- ts/nelm-types/tsconfig.json - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Golangci-lint (Linter) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at ilya.lesikov@flant.com.
Documentation & Help
AI Code Review powered by
ts/nelm-types/src/index.ts
Outdated
| Icon: string; | ||
| APIVersion: string; | ||
| Condition: string; | ||
| Tags: string; |
There was a problem hiding this comment.
The Tags field is incorrectly typed as string, but Helm Chart.yaml specifies tags as a list of strings, so it should be string[] to match the expected data structure.
Code Review Run #aee90b
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
internal/chart/chart_render.go
Outdated
| } | ||
|
|
||
| if featgate.FeatGateTypescript.Enabled() { | ||
| jsRenderedTemplates, err := renderJSTemplates(ctx, originalChartPath, chart, renderedValues) |
There was a problem hiding this comment.
For remote charts, originalChartPath is the remote URL, but TypeScript rendering requires the local filesystem path where the chart is downloaded. Use chartPath instead, which is updated by downloadChart to point to the local directory.
Code suggestion
Check the AI-generated fix before applying
--- a/internal/chart/chart_render.go
+++ b/internal/chart/chart_render.go
@@ -212,1 +212,1 @@
- jsRenderedTemplates, err := renderJSTemplates(ctx, originalChartPath, chart, renderedValues)
+ jsRenderedTemplates, err := renderJSTemplates(ctx, chartPath, chart, renderedValues)
Code Review Run #aee90b
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| "description": "TypeScript chart for %s", | ||
| "main": "src/index.ts", | ||
| "scripts": { | ||
| "build": "npx tsc --noEmit", |
There was a problem hiding this comment.
The build script only type-checks without emitting compiled files, which doesn't match standard build expectations and ignores the tsconfig.json outDir setting. This could confuse users expecting a working build.
Code Review Run #aee90b
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
ts/nelm-types/package.json
Outdated
| @@ -0,0 +1,30 @@ | |||
| { | |||
| "name": "@nelm/types", | |||
There was a problem hiding this comment.
@DmitryBochkarev @ilya-lesikov
npm package is ready - https://www.npmjs.com/package/@nelm/chart-ts-sdk
So we can remove ts/nelm-types content, it was moved into the package
37f16e7 to
5d31dbb
Compare
Signed-off-by: Dmitry Bochkarev <dimabochkarev@gmail.com>
5d31dbb to
d915dc2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d915dc2166
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if parentVals, ok := parentValues["Values"]; ok { | ||
| switch v := parentVals.(type) { | ||
| case map[string]interface{}: | ||
| if subVals, ok := v[subchartName]; ok { | ||
| scoped["Values"] = subVals |
There was a problem hiding this comment.
Preserve global values when scoping TS subcharts
When rendering TypeScript subcharts, scopeValuesForSubchart only passes Values[subchartName] and drops top-level Values.global. In Helm, subcharts inherit .Values.global, so any TS subchart that reads context.Values.global.* (or relies on shared global settings) will now see an empty map and render incorrect manifests. Consider merging the parent global map into the scoped values before returning to keep behavior consistent with Helm’s subchart value scoping.
Useful? React with 👍 / 👎.
|
@DmitryBochkarev Thanks a lot, great PR! That was a lot of work. I'm gonna finish it up, e.g. there is some stuff to do to make it compatible with werf, and we also need some docs, and then we'll announce it. |
This PR is based on #497 and #500
Closes #54
I plan to provide more detailed examples in https://github.com/DmitryBochkarev/nelm-ts-examples
This is quite a big PR. I can try to split it into separate commits. 1000 lines of 2000 all implementation code are TS project initialization code(
nelm chart create), and 2/3 of the rest of the code is tests - so 1,000 lines of actual feature implementation.This PR is slightly diverged from original proposal regarding target js bundle file name. I named it
ts/chart_render_main.jsso it is explicit and hard to randomly create such file.TypeScript types a more simple than propose in #500
but it can be fixed later.
What is implemented
What is not implemented
What I need help with