Bundle Analysis: update module <-> changed files matching heuristic (#488)

Author: JerrySentry

shared/bundle_analysis/report.py

@@ -103,17 +103,33 @@ def modules(
                 .filter(Asset.id == self.asset.id)
             )
 
-            # Apply additional filter if pr_changed_files is provided
-            if pr_changed_files:
-                # Normalize pr_changed_files to have './' prefix for relative paths
-                normalized_files = {
-                    f"./{file}" if not file.startswith("./") else file
-                    for file in pr_changed_files
-                }
-                query = query.filter(Module.name.in_(normalized_files))
-
-            modules = query.all()
-        return [ModuleReport(self.db_path, module) for module in modules]
+            if pr_changed_files is None:
+                return [ModuleReport(self.db_path, module) for module in query]
+
+            """
+            Apply filter on modules where the module name has to be part of the PR's changed file list.
+            However we can't simply do a simple equality match because the module names we store is relative
+            to the root of the app while the PR's file path is relative to the root of the repo. So we will
+            need to check that any of the PR's file lists ends with any of the modules for the given asset.
+            For example,
+                PR changed files: ["abc/def.ts", "ghi/jkl.ts"],
+                modules: ["def.ts", "mno.ts"]
+                -> ["def.ts"]
+            """
+            normalized_changed_files = [
+                os.path.normpath(path[2:] if path.startswith("./") else path)
+                for path in pr_changed_files
+            ]
+            filtered_modules = set()
+            for file in normalized_changed_files:
+                for module in query:
+                    normalized_module = os.path.normpath(
+                        module.name[1:] if file.startswith(".") else module.name
+                    )
+                    if file.endswith(normalized_module):
+                        filtered_modules.add(module)
+
+            return [ModuleReport(self.db_path, module) for module in filtered_modules]
 
     def routes(self) -> Optional[List[str]]:
         plugin_name = self.bundle_info.get("plugin_name")

tests/unit/bundle_analysis/test_bundle_comparison.py

@@ -324,22 +324,33 @@ def test_bundle_asset_comparison_using_closest_size_delta():
     ].contributing_modules()
     assert [module.name for module in module_reports] == []
 
-    # Check asset contributing modules is correct when pr_files are used for filtering
+    # Check no contributing modules filter
+    module_reports = asset_comparison_d[
+        ("assets/index-666d2e09.js", "assets/index-666d2e09.js")
+    ].contributing_modules()
+    assert len(module_reports) == 28
+
+    # Check no PR changed files
+    module_reports = asset_comparison_d[
+        ("assets/index-666d2e09.js", "assets/index-666d2e09.js")
+    ].contributing_modules([])
+    assert len(module_reports) == 0
+
+    # Check with proper filtered files
     module_reports = asset_comparison_d[
         ("assets/index-666d2e09.js", "assets/index-666d2e09.js")
     ].contributing_modules(
-        pr_changed_files=[
-            "src/index.css",
-            "src/main.tsx",
-            "./index.html",
-            "/src/App.css",
-        ]  # first 3 is in, 4th is not
+        [
+            "app1/index.html",
+            "app2/index.html",  # <- don't match because dupe
+            "./app1/src/main.tsx",
+            "/example/svelte/app1/src/App.css",
+            "abc/def/ghi.ts",  # <- don't match
+        ]
+    )
+    assert set([module.name for module in module_reports]) == set(
+        ["./index.html", "./src/App.css", "./src/main.tsx"]
     )
-    assert [module.name for module in module_reports] == [
-        "./src/index.css",
-        "./src/main.tsx",
-        "./index.html",
-    ]
 
 
 def test_bundle_asset_comparison_using_uuid():