Add support for importing: yaml, json, txt, png, gif, jpg, svg, webp, md, csv, pdf, sql, xml #452
Conversation
WalkthroughAdds esbuild asset importers (YAML/JSON/Text/File), a pre-typecheck step that ensures/patches a TypeScript declaration file, adds "fsevents" to esbuild externals, includes tests for importer/declaration behavior, and applies a whitespace-only test alignment change. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI/Runner
participant B as Bundler
participant FS as Filesystem
participant EB as esbuild
participant P as ImporterPlugins
CLI->>B: Build(dir)
B->>FS: possiblyCreateDeclarationFile(node_modules/@types/agentuity)
FS-->>B: created / already up-to-date
B->>EB: Build({ externals: ["bun","fsevents"], plugins: [core, YAML, JSON, Text, File] })
EB->>P: OnResolve(request for *.yaml|*.json|*.txt|*.png|...)
P-->>EB: Resolved(path, namespace)
EB->>P: OnLoad(namespace, path)
alt structured/text asset
P-->>EB: Emit JS module (export default JSON/string)
else binary asset
P-->>EB: Emit JS module (export default Uint8Array(base64))
end
EB-->>B: Bundle result
B-->>CLI: Output / Errors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
internal/bundler/importers.go (1)
126-128: Emit ESM default for binary assets.Use export default to avoid CommonJS interop quirks under FormatESModule.
Apply this diff:
- js := "module.exports = Buffer.from('" + base64Data + "', 'base64');" + js := "export default Buffer.from('" + base64Data + "', 'base64');"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/bundler/bundler.go(3 hunks)internal/bundler/importers.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/bundler/importers.go (1)
internal/util/io.go (1)
Exists(14-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
internal/bundler/bundler.go (3)
283-285: Good pre-typecheck step.Creating the declaration file before running tsc is the right call to avoid missing module type errors.
355-355: Externals look correct.Marking bun and fsevents as externals prevents accidental bundling of runtime/optional deps.
359-365: Plugin wiring LGTM.Importers are correctly registered after the base plugin. See importers.go comments for minor fixes inside the plugins.
internal/bundler/importers.go (4)
64-77: Replicate the resolve fix here (JSON OnResolve).Same absolute path and node_modules detection issues as YAML.
See the YAML OnResolve diff and apply the same pattern.
104-118: Replicate the resolve fix here (File OnResolve).Same cross‑platform/node_modules handling as above.
Apply the YAML OnResolve pattern.
171-190: Declaration helper LGTM.Idempotent and logs actions; placed before typecheck in bundler.go. Good.
192-242: Type declarations look comprehensive.Covers all targeted extensions; aligns with the importer set.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/bundler/importers.go (2)
59-98: Consider relying on esbuild’s built‑in JSON loader.Esbuild already supports importing JSON as ESM. Unless you need custom filtering/logging, removing this plugin reduces maintenance.
192-242: Type declarations: ensure Node types are available and consider broader coverage.
- Buffer type requires @types/node (or TS lib "node") in builds that consume these declarations.
- Optional: add .avif/.ico if needed; consider exporting SVG as string when used as markup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/bundler/bundler_test.go(1 hunks)internal/bundler/importers.go(1 hunks)internal/envutil/envutil_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- internal/envutil/envutil_test.go
- internal/bundler/bundler_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/bundler/importers.go (1)
internal/util/io.go (1)
Exists(14-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test CLI Upgrade Path (windows-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
internal/bundler/importers.go (9)
38-52: YAML OnLoad: good switch to ESM default and generic decode.Correctly supports non-object roots and emits ESM.
Please confirm cstr.JSONStringify safely handles YAML maps with non-string keys (map[any]any). If not, we should normalize keys to strings before stringifying.
79-93: JSON OnLoad: ESM default and generic decode look good.
120-129: Confirm runtime provides global Buffer.If bundling for a browser target, Buffer may not exist; you’d need a polyfill or export a base64/data URL instead.
156-164: Text OnLoad: correct UTF‑8 string export via ESM.
171-190: Declaration file creation flow looks solid.
23-36: Fix absolute-path resolution and cross‑platform node_modules check (YAML OnResolve).Use filepath.IsAbs and filepath.ToSlash; current logic can mis-resolve and fails on Windows.
Apply this diff:
- build.OnResolve(api.OnResolveOptions{Filter: filter, Namespace: "file"}, func(args api.OnResolveArgs) (api.OnResolveResult, error) { - p := args.Path - abs, err := filepath.Abs(p) - if err != nil { - return api.OnResolveResult{}, err - } - if abs != p { - p = filepath.Join(args.ResolveDir, p) - } - if strings.Contains(p, "node_modules/") { - return api.OnResolveResult{}, nil - } - return api.OnResolveResult{Path: p, Namespace: "yaml"}, nil - }) + build.OnResolve(api.OnResolveOptions{Filter: filter, Namespace: "file"}, func(args api.OnResolveArgs) (api.OnResolveResult, error) { + p := args.Path + if !filepath.IsAbs(p) { + p = filepath.Join(args.ResolveDir, p) + } else { + p = filepath.Clean(p) + } + if strings.Contains(filepath.ToSlash(p), "/node_modules/") { + return api.OnResolveResult{}, nil + } + return api.OnResolveResult{Path: p, Namespace: "yaml"}, nil + })
141-154: Fix absolute-path resolution and cross‑platform node_modules check (Text OnResolve).Namespace fix is good; path and node_modules checks still need hardening.
Apply this diff:
- build.OnResolve(api.OnResolveOptions{Filter: filter, Namespace: "file"}, func(args api.OnResolveArgs) (api.OnResolveResult, error) { - p := args.Path - abs, err := filepath.Abs(p) - if err != nil { - return api.OnResolveResult{}, err - } - if abs != p { - p = filepath.Join(args.ResolveDir, p) - } - if strings.Contains(p, "node_modules/") { - return api.OnResolveResult{}, nil - } - return api.OnResolveResult{Path: p, Namespace: "text"}, nil - }) + build.OnResolve(api.OnResolveOptions{Filter: filter, Namespace: "file"}, func(args api.OnResolveArgs) (api.OnResolveResult, error) { + p := args.Path + if !filepath.IsAbs(p) { + p = filepath.Join(args.ResolveDir, p) + } else { + p = filepath.Clean(p) + } + if strings.Contains(filepath.ToSlash(p), "/node_modules/") { + return api.OnResolveResult{}, nil + } + return api.OnResolveResult{Path: p, Namespace: "text"}, nil + })
105-118: Fix absolute-path resolution and cross‑platform node_modules check (File OnResolve).Same issue as above.
Apply this diff:
- build.OnResolve(api.OnResolveOptions{Filter: filter, Namespace: "file"}, func(args api.OnResolveArgs) (api.OnResolveResult, error) { - p := args.Path - abs, err := filepath.Abs(p) - if err != nil { - return api.OnResolveResult{}, err - } - if abs != p { - p = filepath.Join(args.ResolveDir, p) - } - if strings.Contains(p, "node_modules/") { - return api.OnResolveResult{}, nil - } - return api.OnResolveResult{Path: p, Namespace: "file"}, nil - }) + build.OnResolve(api.OnResolveOptions{Filter: filter, Namespace: "file"}, func(args api.OnResolveArgs) (api.OnResolveResult, error) { + p := args.Path + if !filepath.IsAbs(p) { + p = filepath.Join(args.ResolveDir, p) + } else { + p = filepath.Clean(p) + } + if strings.Contains(filepath.ToSlash(p), "/node_modules/") { + return api.OnResolveResult{}, nil + } + return api.OnResolveResult{Path: p, Namespace: "file"}, nil + })
64-77: Fix absolute-path resolution and cross‑platform node_modules check (JSON OnResolve).Same issue as YAML.
Apply this diff:
- build.OnResolve(api.OnResolveOptions{Filter: filter, Namespace: "file"}, func(args api.OnResolveArgs) (api.OnResolveResult, error) { - p := args.Path - abs, err := filepath.Abs(p) - if err != nil { - return api.OnResolveResult{}, err - } - if abs != p { - p = filepath.Join(args.ResolveDir, p) - } - if strings.Contains(p, "node_modules/") { - return api.OnResolveResult{}, nil - } - return api.OnResolveResult{Path: p, Namespace: "json"}, nil - }) + build.OnResolve(api.OnResolveOptions{Filter: filter, Namespace: "file"}, func(args api.OnResolveArgs) (api.OnResolveResult, error) { + p := args.Path + if !filepath.IsAbs(p) { + p = filepath.Join(args.ResolveDir, p) + } else { + p = filepath.Clean(p) + } + if strings.Contains(filepath.ToSlash(p), "/node_modules/") { + return api.OnResolveResult{}, nil + } + return api.OnResolveResult{Path: p, Namespace: "json"}, nil + })
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/bundler/importers.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/bundler/importers.go (1)
internal/util/io.go (1)
Exists(14-19)
🔇 Additional comments (7)
internal/bundler/importers.go (7)
28-60: LGTM! YAML importer looks good.The implementation correctly addresses the previous review feedback by using the centralized
makePathhelper, decoding into a genericanytype to handle non-object roots, and emitting ESM exports.
62-94: LGTM! JSON importer looks good.The implementation correctly addresses the previous review feedback by using the centralized
makePathhelper, decoding into a genericanytype to handle non-object roots, and emitting ESM exports.
96-123: LGTM! File importer implementation is correct.The binary file handling correctly encodes data to base64 and reconstructs it as a Buffer in JavaScript, which is appropriate for binary assets.
125-151: LGTM! Text importer looks good.The implementation correctly addresses the previous review feedback by using the centralized
makePathhelper, converting bytes to UTF-8 string, and emitting ESM exports.
18-26: LGTM! Good refactoring of path resolution logic.The centralized
makePathhelper eliminates code duplication and correctly handles absolute/relative path resolution across all importers.
153-172: LGTM! Declaration file creation logic is solid.The function correctly handles directory creation, file existence checks, and provides good error context. The use of
sys.Existsis appropriate and consistent with the codebase patterns.
174-224: LGTM! TypeScript declarations are comprehensive.The declarations correctly cover all supported file types with appropriate TypeScript types (
anyfor YAML/JSON,Bufferfor binary assets,stringfor text).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
internal/bundler/importers.go (6)
36-36: Make extension filters case‑insensitive (handles .PNG/.YAML, etc.).Regex filters are currently case‑sensitive; imports with uppercase extensions will be missed on case‑insensitive FS. Recommend using (?i) and simplifying jpeg.
Apply this diff:
- filter := "\\.ya?ml$" + filter := "(?i)\\.ya?ml$" @@ - filter := "\\.json$" + filter := "(?i)\\.json$" @@ - filter := "\\.(gif|png|jpg|jpeg|svg|webp)$" + filter := "(?i)\\.(gif|png|jpe?g|svg|webp)$" @@ - filter := "\\.(txt)$" + filter := "(?i)\\.txt$"Also applies to: 70-70, 104-104, 133-133
45-59: YAML OnLoad: consider supporting multi‑document YAML files.Current implementation decodes only the first document. If multi-doc YAML is a possibility, export an array when multiple docs are present.
Apply this diff and add the imports noted below:
- var kv any - err = yaml.NewDecoder(of).Decode(&kv) - if err != nil { - return api.OnLoadResult{}, err - } - js := "export default " + cstr.JSONStringify(kv) + dec := yaml.NewDecoder(of) + var docs []any + for { + var v any + if err := dec.Decode(&v); err != nil { + if errors.Is(err, io.EOF) { + break + } + return api.OnLoadResult{}, err + } + docs = append(docs, v) + } + var out any + if len(docs) == 1 { + out = docs[0] + } else { + out = docs + } + js := "export default " + cstr.JSONStringify(out)Also add these imports at the top:
import ( "io" "errors" )
142-150: Text OnLoad: correct UTF‑8 export via ESM default.Assuming UTF‑8 for .txt is reasonable. If BOMs are expected, consider stripping them.
157-176: Declaration file creation: idempotent and with clear logs.Looks good. Minor: consider writing atomically (write temp + rename) to avoid partial files if the process is interrupted.
178-178: Add Node types reference to support Buffer without requiring tsconfig/types wiring.The declarations refer to Buffer; adding a triple‑slash reference helps TS find Node’s types even if the project’s tsconfig doesn’t include @types/node explicitly.
Apply this diff:
-var declaration = ` +var declaration = ` +/// <reference types="node" />
179-192: Considerunknowninstead ofanyfor YAML/JSON module types.This preserves type safety while still allowing consumers to narrow types. Optional if you prefer maximum permissiveness.
Apply this diff:
-declare module '*.yml' { - const value: any; +declare module '*.yml' { + const value: unknown; export default value; } @@ -declare module '*.yaml' { - const value: any; +declare module '*.yaml' { + const value: unknown; export default value; } @@ -declare module '*.json' { - const value: any; +declare module '*.json' { + const value: unknown; export default value; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/bundler/bundler.go(3 hunks)internal/bundler/importers.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/bundler/bundler.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/bundler/importers.go (1)
internal/util/io.go (1)
Exists(14-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and Test (macos-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
internal/bundler/importers.go (7)
18-31: Helper utilities look solid and cross‑platform‑safe.makePath and isNodeModulesPath correctly handle absolute vs. relative paths and normalize separators for Windows. Nice extraction to reduce duplication.
37-43: YAML OnResolve: correct namespace routing and node_modules bypass.This will correctly route only non-node_modules .yml/.yaml to the YAML loader.
71-77: JSON OnResolve: namespace and node_modules handling look good.
79-93: JSON OnLoad emits ESM default and supports non‑object roots.Good alignment with ESM and root array/scalar support.
113-121: Using Node’s Buffer ties output to platform=node; confirm build target.Since the generated module references the global Buffer, ensure esbuild is configured with Platform=node and that runtime is Node (not browser/neutral). Otherwise this will fail or pull polyfills.
Can you confirm BuildOptions.Platform is set to node where these plugins are used?
134-140: Text OnResolve: correct namespace and node_modules skip.
194-227: Image/text declarations align with runtime exports. Verify Node types availability.Types for image modules as Buffer and text as string align with plugin outputs. With the Node reference added above, TS should resolve Buffer. If you choose not to add the reference, ensure @types/node is included via tsconfig.
jhaynie
changed the title
jhaynie
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
internal/bundler/importers.go (5)
45-59: Support multi‑document YAML (emit array when >1 doc).Current decode reads only the first document. Many YAMLs are multi-doc; emitting an array preserves all content.
Apply this diff:
- var kv any - err = yaml.NewDecoder(of).Decode(&kv) - if err != nil { - return api.OnLoadResult{}, err - } - js := "export default " + cstr.JSONStringify(kv) + dec := yaml.NewDecoder(of) + var docs []any + for { + var v any + if err := dec.Decode(&v); err != nil { + if errors.Is(err, io.EOF) { + break + } + return api.OnLoadResult{}, err + } + docs = append(docs, v) + } + var out any + if len(docs) == 1 { + out = docs[0] + } else { + out = docs + } + js := "export default " + cstr.JSONStringify(out)Add needed imports:
import ( "errors" "io" )
100-112: Binary importer uses Node’s Buffer; confirm Node‑only target or consider data URLs.Buffer isn’t available in browsers without polyfills. If this bundler can target browser runtimes, export a data URL string instead.
Optional change to emit a data URL (keeps base64, broadens runtime compatibility):
base64Data := base64.StdEncoding.EncodeToString(data) - js := "export default Buffer.from(" + cstr.JSONStringify(base64Data) + ", 'base64');" + mt := mime.TypeByExtension(strings.ToLower(filepath.Ext(args.Path))) + if mt == "" { + mt = "application/octet-stream" + } + js := "export default " + cstr.JSONStringify("data:"+mt+";base64,"+base64Data)Add import:
import "mime"Note: If you adopt data URLs, update the declaration types for these extensions from Buffer to string.
Also applies to: 113-122
36-36: Make extension filters case‑insensitive.Prevents misses on uppercase extensions (e.g., .PNG, .JSON).
- filter := "\\.ya?ml$" + filter := "(?i)\\.ya?ml$"- filter := "\\.json$" + filter := "(?i)\\.json$"- filter := "\\.(gif|png|jpg|jpeg|svg|webp|pdf)$" + filter := "(?i)\\.(gif|png|jpg|jpeg|svg|webp|pdf)$"- filter := "\\.(txt|md|csv|xml|sql)$" + filter := "(?i)\\.(txt|md|csv|xml|sql)$"Also applies to: 70-70, 104-104, 133-133
142-149: Handle UTF‑8 BOM and normalize text (optional).Trim BOM to avoid stray U+FEFF and optionally normalize CRLF.
data, err := os.ReadFile(args.Path) if err != nil { return api.OnLoadResult{}, err } - js := "export default " + cstr.JSONStringify(string(data)) + // Trim UTF-8 BOM if present + data = bytes.TrimPrefix(data, []byte{0xEF, 0xBB, 0xBF}) + // Optionally normalize CRLF -> LF + // data = bytes.ReplaceAll(data, []byte("\r\n"), []byte("\n")) + js := "export default " + cstr.JSONStringify(string(data))Add import:
import "bytes"
178-183: Reference Node types so Buffer is typed without extra tsconfig.Prevents “Cannot find name 'Buffer'” in projects not already including @types/node.
var declaration = ` +/// <reference types="node" /> declare module '*.yml' {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/bundler/importers.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/bundler/importers.go (1)
internal/util/io.go (1)
Exists(14-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
internal/bundler/importers.go (4)
18-26: Path normalization helper looks solid.Good use of filepath.IsAbs + Clean/Join; keeps resolution deterministic.
28-30: Cross‑platform node_modules detection LGTM.Using filepath.ToSlash avoids Windows path pitfalls.
32-44: YAML OnResolve registration is correct.Resolving from "file" into a custom "yaml" namespace with node_modules skip is the right shape.
157-176: Declaration file creation flow is clean and idempotent.Good existence check, MkdirAll, and explicit error wrapping.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/bundler/importers.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/bundler/importers.go (1)
internal/util/io.go (1)
Exists(14-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
internal/bundler/importers.go (8)
18-30: Solid cross‑platform path handling.makePath and isNodeModulesPath correctly normalize absolute/relative paths and handle Windows separators.
37-43: YAML OnResolve looks correct.Resolves relative vs absolute and skips node_modules as intended.
45-59: YAML OnLoad: ESM default and generic root supported.Decoding into any and exporting default is the right call.
71-77: JSON OnResolve looks correct.Consistent with YAML and avoids node_modules.
79-93: JSON OnLoad: Good switch to any + ESM default.This will handle arrays/scalars and align with ESM.
133-140: Text OnResolve looks correct.Namespace and node_modules guard are right.
142-150: Text OnLoad: correct UTF‑8 string export via ESM.Straightforward and safe.
188-263: Type declarations cover all added extensions and match runtime shapes.Binary as Uint8Array and text/json/yaml as string/any looks consistent with the importers.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/bundler/importers.go (1)
37-43: De‑duplicate identical OnResolve logic with a small helper.All four blocks share the same resolver body; extracting a helper reduces repetition and drift.
Example helper (placed above plugin creators):
func makeResolver(ns string) func(api.OnResolveArgs) (api.OnResolveResult, error) { return func(args api.OnResolveArgs) (api.OnResolveResult, error) { p := makePath(args) if isNodeModulesPath(p) { return api.OnResolveResult{}, nil } return api.OnResolveResult{Path: p, Namespace: ns}, nil } }Then:
build.OnResolve(api.OnResolveOptions{Filter: filter, Namespace: "file"}, makeResolver("yaml")) // ... json build.OnResolve(api.OnResolveOptions{Filter: filter, Namespace: "file"}, makeResolver("json")) // ... file build.OnResolve(api.OnResolveOptions{Filter: filter, Namespace: "file"}, makeResolver("file")) // ... text build.OnResolve(api.OnResolveOptions{Filter: filter, Namespace: "file"}, makeResolver("text"))Also applies to: 71-77, 105-111, 134-140
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/bundler/importers.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/bundler/importers.go (1)
internal/util/io.go (1)
Exists(14-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test CLI Upgrade Path (windows-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/bundler/importers.go (2)
18-31: Cross‑platform path handling looks good.makePath and isNodeModulesPath correctly normalize and detect node_modules across OSes. Nice cleanup.
113-121: Replace atob with Buffer for Node compatibility (binary importer).atob is undefined in Node; importing binary assets will crash at runtime. Use Buffer.from with a browser fallback.
Apply this diff:
- base64Data := base64.StdEncoding.EncodeToString(data) - js := "export default new Uint8Array(atob(" + cstr.JSONStringify(base64Data) + ").split('').map(c => c.charCodeAt(0)));" + base64Data := base64.StdEncoding.EncodeToString(data) + js := "const __b64=" + cstr.JSONStringify(base64Data) + ";" + + "export default (typeof Buffer!=='undefined'" + + "? new Uint8Array(Buffer.from(__b64,'base64'))" + + ": new Uint8Array(atob(__b64).split('').map(c=>c.charCodeAt(0))));"Run to spot any other atob usage:
#!/bin/bash rg -nP '\batob\s*\(' -C2 --type=go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/bundler/importers.go (1)
142-150: Optional: strip UTF‑8 BOM in text files.Prevents stray U+FEFF at start of strings from BOM’d files.
Apply this diff:
- js := "export default " + cstr.JSONStringify(string(data)) + text := string(data) + if strings.HasPrefix(text, "\uFEFF") { + text = strings.TrimPrefix(text, "\uFEFF") + } + js := "export default " + cstr.JSONStringify(text)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/bundler/importers.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/bundler/importers.go (1)
internal/util/io.go (1)
Exists(14-19)
🔇 Additional comments (4)
internal/bundler/importers.go (4)
18-30: Good cross‑platform path handling.makePath and isNodeModulesPath correctly handle absolute paths and path separators across OSes.
45-59: YAML importer: supports non‑object roots and emits ESM.Decoding into any and exporting default JS is correct.
79-93: JSON importer: supports non‑object roots and emits ESM.Decoding into any and exporting default JS is correct.
113-121: Replace atob with Buffer (add browser fallback) to avoid Node runtime errors.atob is not defined in Node; this will crash in CLI builds. Use Buffer.from with a browser fallback.
Apply this diff:
- base64Data := base64.StdEncoding.EncodeToString(data) - js := "export default new Uint8Array(atob(" + cstr.JSONStringify(base64Data) + ").split('').map(c => c.charCodeAt(0)));" + base64Data := base64.StdEncoding.EncodeToString(data) + js := "{" + + "const b64=" + cstr.JSONStringify(base64Data) + ";" + + "const bytes=(typeof Buffer!=='undefined'&&Buffer.from" + + ")?new Uint8Array(Buffer.from(b64,'base64'))" + + ":new Uint8Array(Array.from(atob(b64),c=>c.charCodeAt(0)));" + + "export default bytes;" + + "}"
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/bundler/importers.go (3)
118-121: Make binary importer work in both Node and browser (Buffer fallback).If this ever targets browser bundles, Buffer is undefined and this will crash. Add a safe fallback to atob.
Is the bundler strictly Node‑only? If not, apply:
- base64Data := base64.StdEncoding.EncodeToString(data) - js := "export default new Uint8Array(Buffer.from(" + cstr.JSONStringify(base64Data) + ", \"base64\"));" + base64Data := base64.StdEncoding.EncodeToString(data) + b64 := cstr.JSONStringify(base64Data) + js := "const __b64 = " + b64 + ";" + js += "export default (typeof Buffer !== 'undefined' ? new Uint8Array(Buffer.from(__b64, 'base64')) : (function(){var bin=atob(__b64);var arr=new Uint8Array(bin.length);for (var i=0;i<bin.length;i++) arr[i]=bin.charCodeAt(i);return arr;})());"
143-149: Strip UTF‑8 BOM for text assets.Prevents invisible leading char issues in consumers.
- js := "export default " + cstr.JSONStringify(string(data)) + text := strings.TrimPrefix(string(data), "\uFEFF") + js := "export default " + cstr.JSONStringify(text)
51-57: YAML reads only the first document. Consider multi‑doc support.If users provide multi‑document YAML (--- ...), decode all docs and export a single value (first doc or array of docs).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/bundler/importers.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/bundler/importers.go (1)
internal/util/io.go (1)
Exists(14-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test CLI Upgrade Path (macos-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/bundler/importers.go (2)
18-30: Cross‑platform path handling looks solid.makePath and isNodeModulesPath correctly normalize and detect node_modules across OSes.
219-236: Bug: second insertion path gets overwritten; patch may be dropped.contentStr is set with newContent, then immediately reset from newLines, discarding the insertion.
Apply:
- // If we didn't insert it yet, add it after the exports - if !inserted && len(newLines) > 0 { - // Find the position after existing exports - for i, line := range newLines { - if strings.HasPrefix(strings.TrimSpace(line), "export ") { - continue - } - // Insert before the first non-export line - newContent := append(newLines[:i], append([]string{"import './file_types';"}, newLines[i:]...)...) - contentStr = strings.Join(newContent, "\n") - inserted = true - break - } - } - if inserted { - contentStr = strings.Join(newLines, "\n") - } + // If we didn't insert it yet, add it before the first non-export line + if !inserted && len(newLines) > 0 { + for i, line := range newLines { + if strings.HasPrefix(strings.TrimSpace(line), "export ") { + continue + } + newLines = append(newLines[:i], append([]string{"import './file_types';"}, newLines[i:]...)...) + inserted = true + break + } + } + // If inserted in either pass, re-join from newLines + if inserted { + contentStr = strings.Join(newLines, "\n") + }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/bundler/importers.go (4)
52-60: YAML: Support multi‑document files (---) instead of decoding only the first document.Many YAML files contain multiple documents; current decoder reads only the first.
Apply this diff and add imports noted below:
- var kv any - err = yaml.NewDecoder(of).Decode(&kv) - if err != nil { - return api.OnLoadResult{}, err - } - js := "export default " + cstr.JSONStringify(kv) + dec := yaml.NewDecoder(of) + var docs []any + for { + var v any + if err := dec.Decode(&v); err != nil { + if errors.Is(err, io.EOF) { + break + } + return api.OnLoadResult{}, err + } + docs = append(docs, v) + } + var out any + if len(docs) == 1 { + out = docs[0] + } else { + out = docs + } + js := "export default " + cstr.JSONStringify(out)Add imports:
import ( + "errors" + "io" )
119-122: Binary importer: Add browser fallback instead of assuming Node’s Buffer.This currently crashes if output runs in a browser (Buffer is undefined). Add a portable fallback to atob.
Apply:
- js := "export default new Uint8Array(Buffer.from(" + cstr.JSONStringify(base64Data) + ", \"base64\"));" + js := "const __b64=" + cstr.JSONStringify(base64Data) + ";export default (typeof Buffer!=='undefined'?new Uint8Array(Buffer.from(__b64,'base64')):new Uint8Array(Array.from(atob(__b64),c=>c.charCodeAt(0))));"Please confirm whether these assets can be consumed in browser builds. If strictly Node, current code is fine but the fallback is low‑risk.
143-151: Text importer: Strip UTF‑8 BOM to avoid leaking U+FEFF into output.Some editors write BOMs; without stripping, you’ll embed it in the string.
- js := "export default " + cstr.JSONStringify(string(data)) + // Strip UTF-8 BOM if present + if len(data) >= 3 && data[0] == 0xEF && data[1] == 0xBB && data[2] == 0xBF { + data = data[3:] + } + js := "export default " + cstr.JSONStringify(string(data))
165-187: Declaration hash check: Read only the first line instead of a fixed 100 bytes.Fixed-size read can be brittle. Read one line and compare.
- // Read first 100 bytes to check for hash comment - buffer := make([]byte, 100) - n, err := file.Read(buffer) - if err != nil || n == 0 { - return true // Can't read file, needs update - } - - content := string(buffer[:n]) - lines := strings.Split(content, "\n") - if len(lines) == 0 { - return true - } - - // Check if first line contains our hash - firstLine := strings.TrimSpace(lines[0]) + // Read just the first line to check for hash comment + reader := bufio.NewReader(file) + firstLine, err := reader.ReadString('\n') + if err != nil && !errors.Is(err, io.EOF) { + return true // Can't read file, needs update + } + firstLine = strings.TrimSpace(firstLine)Add imports:
import ( + "bufio" + "errors" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/bundler/importers.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/bundler/importers.go (1)
internal/util/io.go (1)
Exists(14-19)
🔇 Additional comments (3)
internal/bundler/importers.go (3)
19-31: Nice cross‑platform path handling helpers.Good extraction of makePath and isNodeModulesPath with filepath.ToSlash; prevents Windows path bugs and keeps OnResolve lean.
33-64: ESM default exports for YAML are consistent and clear.Good choice to standardize on export default and avoid CommonJS; aligns with import usage.
251-281: Patching bug: insertion is overwritten; file_types import may never be persisted.contentStr is reset to join(newLines), discarding the newContent path. This can silently drop the inserted import and break typings.
Apply:
- // If we didn't insert it yet, add it after the exports - if !inserted && len(newLines) > 0 { - // Find the position after existing exports - for i, line := range newLines { - if strings.HasPrefix(strings.TrimSpace(line), "export ") { - continue - } - // Insert before the first non-export line - newContent := append(newLines[:i], append([]string{"import './file_types';"}, newLines[i:]...)...) - contentStr = strings.Join(newContent, "\n") - inserted = true - break - } - } - if inserted { - contentStr = strings.Join(newLines, "\n") - } + // If we didn't insert it yet, add it before the first non-export line + if !inserted && len(newLines) > 0 { + for i, line := range newLines { + if strings.HasPrefix(strings.TrimSpace(line), "export ") { + continue + } + newLines = append(newLines[:i], append([]string{"import './file_types';"}, newLines[i:]...)...) + inserted = true + break + } + } + // If inserted in either pass, re-join from newLines + if inserted { + contentStr = strings.Join(newLines, "\n") + }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
internal/bundler/importers_test.go (2)
104-112: Prefer t.TempDir for simpler temp managementt.TempDir() auto-cleans and reduces boilerplate.
Apply this diff:
- // Create temporary directory and files - tmpDir, err := os.MkdirTemp("", "bundler_test") - if err != nil { - t.Fatalf("failed to create temp dir: %v", err) - } - defer os.RemoveAll(tmpDir) + // Create temporary directory and files + tmpDir := t.TempDir()Also applies to: 119-119
140-167: Add an idempotency check (run twice, ensure no changes on second pass)Catches regressions where the patch might re-insert or reorder lines.
Apply this diff:
// Call the function under test err = possiblyCreateDeclarationFile(mockLog, tmpDir) if err != nil { t.Fatalf("possiblyCreateDeclarationFile failed: %v", err) } // Read the result result, err := os.ReadFile(indexPath) if err != nil { t.Fatalf("failed to read result file: %v", err) } resultStr := string(result) if resultStr != tt.expectedOutput { t.Errorf("unexpected output\nexpected:\n%s\n\nactual:\n%s", tt.expectedOutput, resultStr) } // Verify import detection logic hasImport := strings.Contains(resultStr, "import './file_types'") || strings.Contains(resultStr, "import \"./file_types\"") if tt.shouldInsert && !hasImport { t.Errorf("expected import to be inserted but it wasn't found") } if !tt.shouldInsert && hasImport && !strings.Contains(tt.inputContent, "file_types") { t.Errorf("expected import not to be inserted but it was added") } + + // Idempotency: second run should not change the file + before, _ := os.ReadFile(indexPath) + if err := possiblyCreateDeclarationFile(mockLog, tmpDir); err != nil { + t.Fatalf("second possiblyCreateDeclarationFile failed: %v", err) + } + after, _ := os.ReadFile(indexPath) + if string(before) != string(after) { + t.Errorf("non-idempotent patch: index.d.ts changed on second run") + }internal/bundler/importers.go (3)
101-127: Binary exporter: consider optional browser compatibilityCurrent output relies on Node’s Buffer. If you ever bundle for browsers, guard with a runtime check and fallback to atob.
Apply this diff:
- base64Data := base64.StdEncoding.EncodeToString(data) - js := "export default new Uint8Array(Buffer.from(" + cstr.JSONStringify(base64Data) + ", \"base64\"));" + base64Data := base64.StdEncoding.EncodeToString(data) + js := "const _b64=" + cstr.JSONStringify(base64Data) + ";" + + "const _bytes=typeof Buffer!=='undefined'?Buffer.from(_b64,'base64'):Uint8Array.from(atob(_b64),(c)=>c.charCodeAt(0));" + + "export default new Uint8Array(_bytes);"
158-187: Read the first line instead of a fixed 100 bytesAvoid magic numbers; reading the first line is clearer and more robust to future header changes.
Apply this diff:
func needsDeclarationUpdate(filePath string, expectedHash string) bool { - file, err := os.Open(filePath) - if err != nil { - return true // File doesn't exist or can't be read, needs update - } - defer file.Close() - - // Read first 100 bytes to check for hash comment - buffer := make([]byte, 100) - n, err := file.Read(buffer) - if err != nil || n == 0 { - return true // Can't read file, needs update - } - - content := string(buffer[:n]) - lines := strings.Split(content, "\n") - if len(lines) == 0 { - return true - } - - // Check if first line contains our hash - firstLine := strings.TrimSpace(lines[0]) + f, err := os.Open(filePath) + if err != nil { + return true // File doesn't exist or can't be read, needs update + } + defer f.Close() + + // Read first line to check for hash comment + reader := bufio.NewReader(f) + line, err := reader.ReadString('\n') + if err != nil && !errors.Is(err, io.EOF) { + return true + } + firstLine := strings.TrimSpace(line) expectedPrefix := "// agentuity-types-hash:" if !strings.HasPrefix(firstLine, expectedPrefix) { return true // No hash found, needs update } currentHash := strings.TrimPrefix(firstLine, expectedPrefix) return currentHash != expectedHash }Add imports:
+ "bufio" + "errors" + "io"
29-31: Edge case: handle trailing "node_modules" without slashMinor: Catch paths ending with “…/node_modules” as well.
Apply this diff:
func isNodeModulesPath(p string) bool { - return strings.Contains(filepath.ToSlash(p), "/node_modules/") + s := filepath.ToSlash(p) + return strings.Contains(s, "/node_modules/") || strings.HasSuffix(s, "/node_modules") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/bundler/importers.go(1 hunks)internal/bundler/importers_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/bundler/importers.go (1)
internal/util/io.go (1)
Exists(14-19)
🔇 Additional comments (6)
internal/bundler/importers_test.go (1)
36-102: Solid coverage of insertion scenariosGood parametrization and expected outputs cover the key paths (relative export insertion, no-import case, already-present import, and append-at-end).
internal/bundler/importers.go (5)
33-64: YAML importer implementation looks correctCross‑platform path handling, node_modules exclusion, and ESM default export are all in place.
67-99: JSON importer implementation looks correctMirrors YAML importer with ESM default; node_modules exclusion handled.
143-151: Text importer output is correctEmits UTF‑8 string via JSON stringification as ESM default; good choice.
244-295: Patching logic looks correct and idempotentThe two-pass insert with final guards avoids overwriting and double-insertion. Matches test expectations.
57-59: ESM output confirmed: bundler’s BuildOptions setFormat: api.FormatESModule, so theexport default …loaders are correct.
Example usage:
Summary by CodeRabbit
New Features
Chores
Tests