fix: address review feedback — concurrent-safe consumeAttachments & scope-aware codemod

AlemTuzlak · AlemTuzlak · commit 0aca4e88bc61 · 2026-04-07T12:14:14.000+02:00
1. consumeAttachments: read from a ref mirror instead of side-effecting
   out of a setState updater, avoiding reliance on synchronous updater
   execution under React concurrent mode.

2. Codemod: skip declaration positions (variable, function, class, type,
   interface) and non-reference positions (object keys, member accesses).
   When a local declaration shadows the import name, only rename
   unambiguous type-position references to avoid corrupting unrelated code.
diff --git a/codemods/__tests__/migrate-attachments.test.ts b/codemods/__tests__/migrate-attachments.test.ts
@@ -184,6 +184,38 @@ import { ImageUploadQueue } from "some-other-package";
       expect(output).toContain("ImageUploadQueue");
       expect(output).not.toContain("AttachmentQueue");
     });
+
+    it("does not rename local variables that shadow the import name", () => {
+      const input = `
+import type { ImageUpload } from "@copilotkit/react-ui";
+const x: ImageUpload = {} as any;
+const ImageUpload = "unrelated local variable";
+console.log(ImageUpload);
+`;
+      const output = run(input);
+      // Import and type reference should be renamed
+      expect(output).toContain("import type { Attachment }");
+      expect(output).toContain("const x: Attachment");
+      // Local variable declaration and its reference should NOT be renamed
+      expect(output).toContain('const ImageUpload = "unrelated local variable"');
+      expect(output).toContain("console.log(ImageUpload)");
+    });
+
+    it("does not rename object property keys or member expressions", () => {
+      const input = `
+import type { ImageUpload } from "@copilotkit/react-ui";
+const x: ImageUpload = {} as any;
+const config = { ImageUpload: true };
+const val = obj.ImageUpload;
+`;
+      const output = run(input);
+      // Import and type reference should be renamed
+      expect(output).toContain("import type { Attachment }");
+      expect(output).toContain("const x: Attachment");
+      // Object key and member access should NOT be renamed
+      expect(output).toContain("{ ImageUpload: true }");
+      expect(output).toContain("obj.ImageUpload");
+    });
   });
 
   // -----------------------------------------------------------------------
diff --git a/codemods/migrate-attachments.ts b/codemods/migrate-attachments.ts
@@ -66,26 +66,71 @@ export default function transform(file: FileInfo, api: API) {
         // Rename the imported identifier
         imported.name = newName;
 
-        // NOTE: This rename is not scope-aware — it renames ALL identifiers with the
-        // matching name in the file, not just references to the import. In practice
-        // this is safe because ImageUploadQueue/ImageUpload are unlikely local variable
-        // names, but a scope-aware rename (via jscodeshift's scope utilities) would be
-        // more correct.
-
         // If the local name matched the old imported name (not aliased),
-        // update all references in the file to use the new name.
+        // update references in the file to use the new name.
+        //
+        // To avoid corrupting unrelated code, we check if any local
+        // declaration (variable, function, class) shadows the imported
+        // name. If so, we only rename type-position references (which
+        // unambiguously refer to the type import) and leave value-position
+        // references alone since they may refer to the local binding.
         if (!isAliased) {
+          const hasShadow =
+            root.find(j.VariableDeclarator, { id: { type: "Identifier", name: localName } }).length > 0 ||
+            root.find(j.FunctionDeclaration, { id: { type: "Identifier", name: localName } }).length > 0 ||
+            root.find(j.ClassDeclaration, { id: { type: "Identifier", name: localName } }).length > 0;
+
           root.find(j.Identifier, { name: localName }).forEach((idPath) => {
             // Skip the import specifier itself — already renamed above
             if (idPath.parent.node === spec) return;
-            idPath.node.name = newName;
-          });
 
-          // Also update JSX element names (opening + closing tags)
-          root.find(j.JSXIdentifier, { name: localName }).forEach((idPath) => {
+            const parent = idPath.parent.node;
+
+            // Skip declaration positions — these define new bindings
+            if (parent.type === "VariableDeclarator" && parent.id === idPath.node) return;
+            if (parent.type === "FunctionDeclaration" && parent.id === idPath.node) return;
+            if (parent.type === "ClassDeclaration" && parent.id === idPath.node) return;
+            if (parent.type === "TSTypeAliasDeclaration" && parent.id === idPath.node) return;
+            if (parent.type === "TSInterfaceDeclaration" && parent.id === idPath.node) return;
+
+            // Skip non-computed object property keys and member expression properties
+            if (
+              (parent.type === "Property" || parent.type === "ObjectProperty") &&
+              parent.key === idPath.node &&
+              !parent.computed
+            ) return;
+            if (
+              parent.type === "MemberExpression" &&
+              parent.property === idPath.node &&
+              !parent.computed
+            ) return;
+
+            // Skip import specifiers from other packages
+            if (
+              parent.type === "ImportSpecifier" &&
+              idPath.parent.parent?.node !== path.node
+            ) return;
+
+            // If a local declaration shadows this name, only rename
+            // unambiguous type-position references (e.g. type annotations)
+            if (hasShadow) {
+              const isTypePosition =
+                parent.type === "TSTypeReference" ||
+                parent.type === "TSTypeAnnotation" ||
+                parent.type === "TSTypeQuery";
+              if (!isTypePosition) return;
+            }
+
             idPath.node.name = newName;
           });
 
+          // Only rename JSX identifiers if there's no shadow
+          if (!hasShadow) {
+            root.find(j.JSXIdentifier, { name: localName }).forEach((idPath) => {
+              idPath.node.name = newName;
+            });
+          }
+
           if (spec.local) {
             spec.local.name = newName;
           }
diff --git a/packages/react-core/src/v2/hooks/use-attachments.tsx b/packages/react-core/src/v2/hooks/use-attachments.tsx
@@ -60,10 +60,12 @@ export function useAttachments({
   const fileInputRef = useRef<HTMLInputElement>(null);
   const containerRef = useRef<HTMLDivElement>(null);
 
-  // Keep a ref to the latest config so stable callbacks can read current
-  // values without appearing in dependency arrays.
+  // Keep refs to the latest values so stable callbacks can read current
+  // state without appearing in dependency arrays.
   const configRef = useRef(config);
   configRef.current = config;
+  const attachmentsRef = useRef<Attachment[]>([]);
+  attachmentsRef.current = attachments;
 
   // Stable processFiles — reads config from ref, never changes identity
   const processFiles = useCallback(async (files: File[]) => {
@@ -241,14 +243,10 @@ export function useAttachments({
   }, []);
 
   const consumeAttachments = useCallback(() => {
-    let ready: Attachment[] = [];
-    setAttachments((prev) => {
-      ready = prev.filter((a) => a.status === "ready");
-      if (ready.length === 0) return prev;
-      const remaining = prev.filter((a) => a.status !== "ready");
-      return remaining;
-    });
-    if (ready.length > 0 && fileInputRef.current) {
+    const ready = attachmentsRef.current.filter((a) => a.status === "ready");
+    if (ready.length === 0) return ready;
+    setAttachments((prev) => prev.filter((a) => a.status !== "ready"));
+    if (fileInputRef.current) {
       fileInputRef.current.value = "";
     }
     return ready;
