address PR review comments

- Add bounds check for import_record_index before accessing import records
- Fix memory leak for canonical_refs HashMap keys by freeing them in defer block
- Fix unique_items memory leak by calling deinit when buffer is unused
- Change external_imports_deduplicated flag to only be set when records were actually deduplicated
- Update test regex to handle multi-line imports and optional "type" modifier

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

src/bun.js/bindings/generated_perf_trace_events.h

@@ -16,47 +16,48 @@
   macro(Bundler.breakOutputIntoPieces, 12) \
   macro(Bundler.cloneAST, 13) \
   macro(Bundler.computeChunks, 14) \
-  macro(Bundler.findAllImportedPartsInJSOrder, 15) \
-  macro(Bundler.findReachableFiles, 16) \
-  macro(Bundler.generateChunksInParallel, 17) \
-  macro(Bundler.generateCodeForFileInChunkCss, 18) \
-  macro(Bundler.generateCodeForFileInChunkJS, 19) \
-  macro(Bundler.generateIsolatedHash, 20) \
-  macro(Bundler.generateSourceMapForChunk, 21) \
-  macro(Bundler.markFileLiveForTreeShaking, 22) \
-  macro(Bundler.markFileReachableForCodeSplitting, 23) \
-  macro(Bundler.onParseTaskComplete, 24) \
-  macro(Bundler.postProcessJSChunk, 25) \
-  macro(Bundler.readFile, 26) \
-  macro(Bundler.renameSymbolsInChunk, 27) \
-  macro(Bundler.scanImportsAndExports, 28) \
-  macro(Bundler.treeShakingAndCodeSplitting, 29) \
-  macro(Bundler.writeChunkToDisk, 30) \
-  macro(Bundler.writeOutputFilesToDisk, 31) \
-  macro(ExtractTarball.extract, 32) \
-  macro(FolderResolver.readPackageJSONFromDisk.folder, 33) \
-  macro(FolderResolver.readPackageJSONFromDisk.workspace, 34) \
-  macro(JSBundler.addPlugin, 35) \
-  macro(JSBundler.hasAnyMatches, 36) \
-  macro(JSBundler.matchOnLoad, 37) \
-  macro(JSBundler.matchOnResolve, 38) \
-  macro(JSGlobalObject.create, 39) \
-  macro(JSParser.analyze, 40) \
-  macro(JSParser.parse, 41) \
-  macro(JSParser.postvisit, 42) \
-  macro(JSParser.visit, 43) \
-  macro(JSPrinter.print, 44) \
-  macro(JSPrinter.printWithSourceMap, 45) \
-  macro(ModuleResolver.resolve, 46) \
-  macro(PackageInstaller.install, 47) \
-  macro(PackageManifest.Serializer.loadByFile, 48) \
-  macro(PackageManifest.Serializer.save, 49) \
-  macro(RuntimeTranspilerCache.fromFile, 50) \
-  macro(RuntimeTranspilerCache.save, 51) \
-  macro(RuntimeTranspilerCache.toFile, 52) \
-  macro(StandaloneModuleGraph.serialize, 53) \
-  macro(Symbols.followAll, 54) \
-  macro(TestCommand.printCodeCoverageLCov, 55) \
-  macro(TestCommand.printCodeCoverageLCovAndText, 56) \
-  macro(TestCommand.printCodeCoverageText, 57) \
+  macro(Bundler.deduplicateExternalImports, 15) \
+  macro(Bundler.findAllImportedPartsInJSOrder, 16) \
+  macro(Bundler.findReachableFiles, 17) \
+  macro(Bundler.generateChunksInParallel, 18) \
+  macro(Bundler.generateCodeForFileInChunkCss, 19) \
+  macro(Bundler.generateCodeForFileInChunkJS, 20) \
+  macro(Bundler.generateIsolatedHash, 21) \
+  macro(Bundler.generateSourceMapForChunk, 22) \
+  macro(Bundler.markFileLiveForTreeShaking, 23) \
+  macro(Bundler.markFileReachableForCodeSplitting, 24) \
+  macro(Bundler.onParseTaskComplete, 25) \
+  macro(Bundler.postProcessJSChunk, 26) \
+  macro(Bundler.readFile, 27) \
+  macro(Bundler.renameSymbolsInChunk, 28) \
+  macro(Bundler.scanImportsAndExports, 29) \
+  macro(Bundler.treeShakingAndCodeSplitting, 30) \
+  macro(Bundler.writeChunkToDisk, 31) \
+  macro(Bundler.writeOutputFilesToDisk, 32) \
+  macro(ExtractTarball.extract, 33) \
+  macro(FolderResolver.readPackageJSONFromDisk.folder, 34) \
+  macro(FolderResolver.readPackageJSONFromDisk.workspace, 35) \
+  macro(JSBundler.addPlugin, 36) \
+  macro(JSBundler.hasAnyMatches, 37) \
+  macro(JSBundler.matchOnLoad, 38) \
+  macro(JSBundler.matchOnResolve, 39) \
+  macro(JSGlobalObject.create, 40) \
+  macro(JSParser.analyze, 41) \
+  macro(JSParser.parse, 42) \
+  macro(JSParser.postvisit, 43) \
+  macro(JSParser.visit, 44) \
+  macro(JSPrinter.print, 45) \
+  macro(JSPrinter.printWithSourceMap, 46) \
+  macro(ModuleResolver.resolve, 47) \
+  macro(PackageInstaller.install, 48) \
+  macro(PackageManifest.Serializer.loadByFile, 49) \
+  macro(PackageManifest.Serializer.save, 50) \
+  macro(RuntimeTranspilerCache.fromFile, 51) \
+  macro(RuntimeTranspilerCache.save, 52) \
+  macro(RuntimeTranspilerCache.toFile, 53) \
+  macro(StandaloneModuleGraph.serialize, 54) \
+  macro(Symbols.followAll, 55) \
+  macro(TestCommand.printCodeCoverageLCov, 56) \
+  macro(TestCommand.printCodeCoverageLCovAndText, 57) \
+  macro(TestCommand.printCodeCoverageText, 58) \
   // end

src/bundler/Chunk.zig

@@ -502,6 +502,15 @@ pub const Chunk = struct {
         ///
         /// Mutated while sorting chunks in `computeChunks`
         css_chunks: []u32 = &.{},
+
+        /// When true, external imports have been deduplicated and merged into
+        /// cross_chunk_prefix_stmts. Individual external import statements in
+        /// per-file code should be skipped during code generation.
+        external_imports_deduplicated: bool = false,
+
+        /// Map from import record index to whether it's been deduplicated.
+        /// Used to skip individual external import statements.
+        deduplicated_external_import_records: std.AutoHashMapUnmanaged(u64, void) = .{},
     };
 
     pub const CssChunk = struct {

src/bundler/LinkerContext.zig

@@ -431,6 +431,9 @@ pub const LinkerContext = struct {
 
         try this.computeCrossChunkDependencies(chunks);
 
+        // Deduplicate external imports across files within each chunk
+        this.deduplicateExternalImports(chunks);
+
         if (comptime FeatureFlags.help_catch_memory_issues) {
             this.checkForMemoryCorruption();
         }
@@ -632,6 +635,7 @@ pub const LinkerContext = struct {
     };
 
     pub const computeCrossChunkDependencies = @import("./linker_context/computeCrossChunkDependencies.zig").computeCrossChunkDependencies;
+    pub const deduplicateExternalImports = @import("./linker_context/deduplicateExternalImports.zig").deduplicateExternalImports;
 
     pub const GenerateChunkCtx = struct {
         c: *LinkerContext,

src/bundler/linker_context/convertStmtsForChunk.zig

@@ -74,6 +74,24 @@ pub fn convertStmtsForChunk(
                 .s_import => |s| {
                     // "import * as ns from 'path'"
                     // "import {foo} from 'path'"
+
+                    // Check if this external import has been deduplicated
+                    if (chunk.content == .javascript) {
+                        const js_chunk = &chunk.content.javascript;
+                        if (js_chunk.external_imports_deduplicated) {
+                            // Bounds check before accessing import record
+                            if (s.import_record_index >= ast.import_records.len) continue;
+                            const record = ast.import_records.at(s.import_record_index);
+                            if (!record.source_index.isValid() and record.kind == .stmt and !record.path.is_disabled) {
+                                const record_key = (@as(u64, source_index) << 32) | @as(u64, s.import_record_index);
+                                if (js_chunk.deduplicated_external_import_records.contains(record_key)) {
+                                    // This external import has been deduplicated - skip it
+                                    continue;
+                                }
+                            }
+                        }
+                    }
+
                     if (try c.shouldRemoveImportExportStmt(
                         stmts,
                         stmt.loc,

src/bundler/linker_context/deduplicateExternalImports.zig

@@ -0,0 +1,257 @@
+/// Deduplicate external imports across files within a chunk.
+/// When multiple files import from the same external module (e.g., "react/jsx-runtime"),
+/// this pass merges the symbol refs so they all point to a canonical symbol.
+/// The first import statement for each path is augmented to include all imports,
+/// and subsequent import statements are removed.
+///
+/// Before:
+///   // a.js
+///   import { jsxDEV } from "react/jsx-dev-runtime";
+///   var X = () => jsxDEV("div", ...);
+///
+///   // index.js
+///   import { jsxDEV as jsxDEV2, Fragment } from "react/jsx-dev-runtime";
+///   var HelloWorld = () => jsxDEV2(Fragment, ...);
+///
+/// After:
+///   // a.js
+///   import { jsxDEV, Fragment } from "react/jsx-dev-runtime";
+///   var X = () => jsxDEV("div", ...);
+///
+///   // index.js
+///   var HelloWorld = () => jsxDEV(Fragment, ...);
+pub fn deduplicateExternalImports(c: *LinkerContext, chunks: []Chunk) void {
+    const trace = bun.perf.trace("Bundler.deduplicateExternalImports");
+    defer trace.end();
+
+    if (!c.options.output_format.keepES6ImportExportSyntax()) {
+        // Only relevant for ESM output where import statements are preserved
+        return;
+    }
+
+    const all_import_records = c.graph.ast.items(.import_records);
+    const all_parts = c.graph.ast.items(.parts);
+
+    for (chunks) |*chunk| {
+        if (chunk.content != .javascript) continue;
+
+        const js_chunk = &chunk.content.javascript;
+        const files_in_order = js_chunk.files_in_chunk_order;
+
+        if (files_in_order.len <= 1) continue; // No deduplication needed for single file
+
+        // Map from (external path, import name) to canonical ref
+        // Key format: "path\x00name" (null-separated)
+        var canonical_refs = std.StringHashMap(CanonicalImport).init(c.allocator());
+        defer {
+            // Free allocated keys before deinit to avoid memory leak
+            var key_iter = canonical_refs.keyIterator();
+            while (key_iter.next()) |key| {
+                c.allocator().free(key.*);
+            }
+            canonical_refs.deinit();
+        }
+
+        // Track the first import statement for each path
+        var canonical_imports_by_path = std.StringHashMap(CanonicalPathImport).init(c.allocator());
+        defer canonical_imports_by_path.deinit();
+
+        // First pass: collect all external imports and establish canonical refs
+        for (files_in_order) |source_index| {
+            const import_records = all_import_records[source_index].slice();
+            const parts = all_parts[source_index].slice();
+
+            for (parts) |part| {
+                for (part.stmts) |stmt| {
+                    if (stmt.data != .s_import) continue;
+                    const s_import = stmt.data.s_import;
+
+                    const record_idx = s_import.import_record_index;
+                    if (record_idx >= import_records.len) continue;
+
+                    const record = &import_records[record_idx];
+
+                    // Only process external ES imports
+                    if (record.source_index.isValid()) continue; // Not external
+                    if (record.kind != .stmt) continue; // Only static imports
+                    if (record.path.is_disabled) continue;
+
+                    const path = record.path.text;
+                    const record_key = (@as(u64, source_index) << 32) | @as(u64, record_idx);
+
+                    // Track if this is the first import record for this path
+                    const path_gop = canonical_imports_by_path.getOrPut(path) catch continue;
+                    const is_canonical_record_for_path = !path_gop.found_existing;
+                    if (is_canonical_record_for_path) {
+                        path_gop.value_ptr.* = .{
+                            .source_index = source_index,
+                            .import_record_index = record_idx,
+                            .s_import = s_import,
+                        };
+                    }
+
+                    // Process each named import item
+                    for (s_import.items) |item| {
+                        const import_name = item.alias;
+                        if (import_name.len == 0) continue;
+
+                        const ref = item.name.ref orelse continue;
+
+                        // Create lookup key: "path\x00import_name"
+                        const key = makeKey(c.allocator(), path, import_name) catch continue;
+
+                        const gop = canonical_refs.getOrPut(key) catch continue;
+                        if (!gop.found_existing) {
+                            // First occurrence - this becomes the canonical ref
+                            gop.value_ptr.* = .{
+                                .canonical_ref = ref,
+                                .alias = import_name,
+                            };
+                        } else {
+                            // Subsequent occurrence - merge to canonical ref
+                            const canonical = gop.value_ptr.canonical_ref;
+                            if (!ref.eql(canonical)) {
+                                _ = c.graph.symbols.merge(ref, canonical);
+                            }
+                        }
+                    }
+
+                    // Handle default import
+                    if (s_import.default_name) |default_name| {
+                        if (default_name.ref) |ref| {
+                            const key = makeKey(c.allocator(), path, "default") catch continue;
+
+                            const gop = canonical_refs.getOrPut(key) catch continue;
+                            if (!gop.found_existing) {
+                                gop.value_ptr.* = .{
+                                    .canonical_ref = ref,
+                                    .alias = "default",
+                                };
+                            } else {
+                                const canonical = gop.value_ptr.canonical_ref;
+                                if (!ref.eql(canonical)) {
+                                    _ = c.graph.symbols.merge(ref, canonical);
+                                }
+                            }
+                        }
+                    }
+
+                    // Handle namespace import (import * as ns)
+                    if (record.flags.contains_import_star) {
+                        const ns_ref = s_import.namespace_ref;
+                        if (ns_ref.isValid()) {
+                            const key = makeKey(c.allocator(), path, "*") catch continue;
+
+                            const gop = canonical_refs.getOrPut(key) catch continue;
+                            if (!gop.found_existing) {
+                                gop.value_ptr.* = .{
+                                    .canonical_ref = ns_ref,
+                                    .alias = "*",
+                                };
+                            } else {
+                                const canonical = gop.value_ptr.canonical_ref;
+                                if (!ns_ref.eql(canonical)) {
+                                    _ = c.graph.symbols.merge(ns_ref, canonical);
+                                }
+                            }
+                        }
+                    }
+
+                    // Mark non-first import records for this path as duplicates (to be removed)
+                    if (!is_canonical_record_for_path) {
+                        js_chunk.deduplicated_external_import_records.put(c.allocator(), record_key, {}) catch continue;
+                    }
+                }
+            }
+        }
+
+        // Second pass: augment the canonical import statements with any additional imports
+        var iter = canonical_imports_by_path.iterator();
+        while (iter.next()) |entry| {
+            const path = entry.key_ptr.*;
+            const canonical_import = entry.value_ptr.*;
+            const s_import = canonical_import.s_import;
+
+            // Find all unique imports for this path
+            var unique_items = std.ArrayListUnmanaged(js_ast.ClauseItem){};
+            var seen_names = std.StringHashMap(void).init(c.allocator());
+            defer seen_names.deinit();
+
+            // First, add existing items
+            for (s_import.items) |item| {
+                if (seen_names.contains(item.alias)) continue;
+                seen_names.put(item.alias, {}) catch continue;
+                unique_items.append(c.allocator(), item) catch continue;
+            }
+
+            // Then, add any additional items from other files
+            var ref_iter = canonical_refs.iterator();
+            while (ref_iter.next()) |ref_entry| {
+                const key = ref_entry.key_ptr.*;
+                const import_info = ref_entry.value_ptr.*;
+
+                // Check if this key is for the current path
+                const null_pos = std.mem.indexOfScalar(u8, key, 0) orelse continue;
+                const key_path = key[0..null_pos];
+                if (!std.mem.eql(u8, key_path, path)) continue;
+
+                const alias = import_info.alias;
+                if (std.mem.eql(u8, alias, "*") or std.mem.eql(u8, alias, "default")) continue;
+
+                if (seen_names.contains(alias)) continue;
+                seen_names.put(alias, {}) catch continue;
+
+                unique_items.append(c.allocator(), .{
+                    .name = .{
+                        .ref = import_info.canonical_ref,
+                        .loc = Logger.Loc.Empty,
+                    },
+                    .alias = alias,
+                    .alias_loc = Logger.Loc.Empty,
+                }) catch continue;
+            }
+
+            // Update the items if we have new ones
+            if (unique_items.items.len > s_import.items.len) {
+                s_import.items = unique_items.items;
+            } else {
+                // Free the unused buffer to avoid memory leak
+                unique_items.deinit(c.allocator());
+            }
+        }
+
+        // Mark chunk as having deduplicated imports only if we actually deduplicated some
+        if (js_chunk.deduplicated_external_import_records.count() > 0) {
+            js_chunk.external_imports_deduplicated = true;
+        }
+    }
+}
+
+const CanonicalImport = struct {
+    canonical_ref: Ref,
+    alias: []const u8,
+};
+
+const CanonicalPathImport = struct {
+    source_index: u32,
+    import_record_index: u32,
+    s_import: *js_ast.S.Import,
+};
+
+fn makeKey(allocator: std.mem.Allocator, path: []const u8, name: []const u8) ![]const u8 {
+    const key = try allocator.alloc(u8, path.len + 1 + name.len);
+    @memcpy(key[0..path.len], path);
+    key[path.len] = 0;
+    @memcpy(key[path.len + 1 ..], name);
+    return key;
+}
+
+const std = @import("std");
+
+const bun = @import("bun");
+const Logger = bun.logger;
+const js_ast = bun.ast;
+
+const Chunk = bun.bundle_v2.Chunk;
+const LinkerContext = bun.bundle_v2.LinkerContext;
+const Ref = bun.bundle_v2.Ref;

src/generated_perf_trace_events.zig

@@ -15,6 +15,7 @@ pub const PerfEvent = enum(i32) {
     @"Bundler.breakOutputIntoPieces",
     @"Bundler.cloneAST",
     @"Bundler.computeChunks",
+    @"Bundler.deduplicateExternalImports",
     @"Bundler.findAllImportedPartsInJSOrder",
     @"Bundler.findReachableFiles",
     @"Bundler.generateChunksInParallel",

test/regression/issue/03029.test.ts

@@ -0,0 +1,136 @@
+import { describe, expect } from "bun:test";
+import { itBundled } from "../../bundler/expectBundled";
+
+describe("issue 3029 - JSX imports should be deduplicated", () => {
+  itBundled("jsx-imports-deduplicated", {
+    files: {
+      "/index.js": `
+        import { X } from "./a.js"
+        const HelloWorld = () => (
+          <>
+            <X/>
+            World
+          </>
+        );
+        export { HelloWorld };
+      `,
+      "/a.js": `
+        const X = () => <div>Hello</div>;
+        export { X };
+      `,
+      // Mock react/jsx-dev-runtime
+      "/node_modules/react/jsx-dev-runtime.js": `
+        export function jsxDEV(type, props, key, source, self) {
+          return { type, props, key, source, self };
+        }
+        export const Fragment = Symbol.for("react.fragment");
+      `,
+    },
+    external: ["react/jsx-dev-runtime"],
+    onAfterBundle(api) {
+      const content = api.readFile("/out.js");
+
+      // Count import statements from react/jsx-dev-runtime
+      // Pattern handles multi-line imports and optional "type" modifier
+      const importMatches = content.match(/import\s*(?:type\s*)?\{[\s\S]*?\}\s*from\s*["']react\/jsx-dev-runtime["']/g);
+
+      // Should have only ONE import statement for react/jsx-dev-runtime
+      expect(importMatches?.length).toBe(1);
+
+      // The single import should contain both jsxDEV and Fragment
+      expect(importMatches?.[0]).toMatch(/jsxDEV/);
+      expect(importMatches?.[0]).toMatch(/Fragment/);
+
+      // Should NOT have jsxDEV2 or any numbered variants
+      expect(content).not.toMatch(/jsxDEV\d/);
+    },
+  });
+
+  itBundled("jsx-imports-deduplicated-multiple-files", {
+    files: {
+      "/index.js": `
+        import { X } from "./a.js"
+        import { Y } from "./b.js"
+        const App = () => (
+          <>
+            <X/>
+            <Y/>
+          </>
+        );
+        export { App };
+      `,
+      "/a.js": `
+        const X = () => <div>X</div>;
+        export { X };
+      `,
+      "/b.js": `
+        const Y = () => <span>Y</span>;
+        export { Y };
+      `,
+      // Mock react/jsx-dev-runtime
+      "/node_modules/react/jsx-dev-runtime.js": `
+        export function jsxDEV(type, props, key, source, self) {
+          return { type, props, key, source, self };
+        }
+        export const Fragment = Symbol.for("react.fragment");
+      `,
+    },
+    external: ["react/jsx-dev-runtime"],
+    onAfterBundle(api) {
+      const content = api.readFile("/out.js");
+
+      // Count import statements from react/jsx-dev-runtime
+      // Pattern handles multi-line imports and optional "type" modifier
+      const importMatches = content.match(/import\s*(?:type\s*)?\{[\s\S]*?\}\s*from\s*["']react\/jsx-dev-runtime["']/g);
+
+      // Should have only ONE import statement
+      expect(importMatches?.length).toBe(1);
+
+      // Should NOT have numbered variants like jsxDEV2, jsxDEV3
+      expect(content).not.toMatch(/jsxDEV\d/);
+    },
+  });
+
+  itBundled("jsx-imports-deduplicated-with-different-imports", {
+    files: {
+      "/index.js": `
+        // This file uses jsxDEV and Fragment
+        import { X } from "./a.js"
+        const HelloWorld = () => (
+          <>
+            <X/>
+          </>
+        );
+        export { HelloWorld };
+      `,
+      "/a.js": `
+        // This file only uses jsxDEV
+        const X = () => <div>Hello</div>;
+        export { X };
+      `,
+      // Mock react/jsx-dev-runtime
+      "/node_modules/react/jsx-dev-runtime.js": `
+        export function jsxDEV(type, props, key, source, self) {
+          return { type, props, key, source, self };
+        }
+        export const Fragment = Symbol.for("react.fragment");
+      `,
+    },
+    external: ["react/jsx-dev-runtime"],
+    onAfterBundle(api) {
+      const content = api.readFile("/out.js");
+
+      // Count import statements from react/jsx-dev-runtime
+      // Pattern handles multi-line imports and optional "type" modifier
+      const importMatches = content.match(/import\s*(?:type\s*)?\{[\s\S]*?\}\s*from\s*["']react\/jsx-dev-runtime["']/g);
+
+      // Should have only ONE import statement
+      expect(importMatches?.length).toBe(1);
+
+      // The single import should contain BOTH jsxDEV and Fragment
+      // (Fragment is used in index.js even though a.js doesn't use it)
+      expect(importMatches?.[0]).toMatch(/jsxDEV/);
+      expect(importMatches?.[0]).toMatch(/Fragment/);
+    },
+  });
+});