SmallStrings insight fixes (#281)

* SmallStrings insight fixes

* Update src/launchpad/size/models/insights.py

Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>

* update tests

* be more conservative

* tests

* tests

---------

Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>

Author: trevor-e

src/launchpad/size/insights/apple/localized_strings.py

@@ -1,36 +1,41 @@
+import re
+
+from pathlib import Path
+
 from launchpad.size.insights.insight import Insight, InsightsInput
-from launchpad.size.models.common import FileInfo
-from launchpad.size.models.insights import FileSavingsResult, LocalizedStringInsightResult
+from launchpad.size.models.insights import LocalizedStringInsightResult
 
 
 class LocalizedStringsInsight(Insight[LocalizedStringInsightResult]):
-    """Insight for analyzing localized strings files in iOS apps."""
+    """Insight for analyzing localized strings files in iOS apps. If the total size of the localized strings files is greater than the threshold, we recommend using our SmallStrings library."""
+
+    SAVINGS_THRESHOLD_BYTES = 100 * 1024  # 100KB
 
-    # 100KB threshold for reporting localized strings
-    THRESHOLD_BYTES = 100 * 1024  # 100KB
+    STRINGS_FILE_DENYLIST = ["LaunchScreen.strings"]
+
+    # Pattern to match .strings files directly inside .lproj directories
+    # Matches: en.lproj/Localizable.strings, Base.lproj/InfoPlist.strings
+    _LPROJ_STRINGS_PATTERN = re.compile(r"[^/]+\.lproj/[^/]+\.strings$")
 
     def generate(self, input: InsightsInput) -> LocalizedStringInsightResult | None:
         """Generate insight for localized strings files.
 
         Finds all Localizable.strings files in *.lproj directories,
         calculates total size, and returns insight if above threshold.
         """
-        localized_files: list[FileInfo] = []
         total_size = 0
 
-        # Find all Localizable.strings files in *.lproj directories
+        # Find all .strings files in *.lproj directories
         for file_info in input.file_analysis.files:
-            # Check if file path ends with *.lproj/Localizable.strings
-            if file_info.path.endswith(".lproj/Localizable.strings"):
-                localized_files.append(file_info)
-                total_size += file_info.size
-
-        if total_size > self.THRESHOLD_BYTES:
-            file_savings = [FileSavingsResult(file_path=file.path, total_savings=file.size) for file in localized_files]
+            if self._LPROJ_STRINGS_PATTERN.search(file_info.path):
+                filename = Path(file_info.path).name
+                if filename not in self.STRINGS_FILE_DENYLIST:
+                    total_size += file_info.size
 
+        small_strings_savings = int(total_size * 0.5)
+        if small_strings_savings > self.SAVINGS_THRESHOLD_BYTES:
             return LocalizedStringInsightResult(
-                files=file_savings,
-                total_savings=total_size,
+                total_savings=small_strings_savings,
             )
 
         return None

src/launchpad/size/models/insights.py

@@ -105,10 +105,11 @@ class WebPOptimizationInsightResult(FilesInsightResult):
     pass
 
 
-class LocalizedStringInsightResult(FilesInsightResult):
+class LocalizedStringInsightResult(BaseInsightResult):
     """Results from localized string analysis.
 
-    Files contain localized strings files exceeding 100KB threshold with their sizes.
+    Reports the total estimated savings that could be achieved by optimizing localized strings.
+
     """
 
     pass

tests/unit/test_localized_strings_insight.py

@@ -18,7 +18,7 @@ def test_generate_with_large_localized_strings(self):
         localized_file_1 = FileInfo(
             full_path=Path("en.lproj/Localizable.strings"),
             path="en.lproj/Localizable.strings",
-            size=60 * 1024,  # 60KB
+            size=150 * 1024,  # 150KB
             file_type="strings",
             treemap_type=TreemapType.RESOURCES,
             hash="hash1",
@@ -27,7 +27,7 @@ def test_generate_with_large_localized_strings(self):
         localized_file_2 = FileInfo(
             full_path=Path("es.lproj/Localizable.strings"),
             path="es.lproj/Localizable.strings",
-            size=50 * 1024,  # 50KB
+            size=60 * 1024,  # 60KB
             file_type="strings",
             treemap_type=TreemapType.RESOURCES,
             hash="hash2",
@@ -56,21 +56,17 @@ def test_generate_with_large_localized_strings(self):
         result = self.insight.generate(insights_input)
 
         assert isinstance(result, LocalizedStringInsightResult)
-        assert len(result.files) == 2
-        assert result.total_savings == 110 * 1024  # 110KB total
-        assert result.files[0].file_path == "en.lproj/Localizable.strings"
-        assert result.files[1].file_path == "es.lproj/Localizable.strings"
-        # Verify savings match file sizes
-        assert result.files[0].total_savings == 60 * 1024
-        assert result.files[1].total_savings == 50 * 1024
+        # Total savings should be 50% of the total file size (210KB * 0.5 = 105KB)
+        expected_total_savings = int((150 + 60) * 1024 * 0.5)
+        assert result.total_savings == expected_total_savings
 
     def test_generate_with_small_localized_strings(self):
-        """Test that no insight is generated when total size is below 100KB threshold."""
-        # Create localized strings files that don't exceed 100KB total
+        """Test that no insight is generated when estimated savings is below 100KB threshold."""
+        # Create localized strings files where estimated savings (50%) don't exceed 100KB
         localized_file_1 = FileInfo(
             full_path=Path("en.lproj/Localizable.strings"),
             path="en.lproj/Localizable.strings",
-            size=40 * 1024,  # 40KB
+            size=60 * 1024,  # 60KB * 0.5 = 30KB savings
             file_type="strings",
             treemap_type=TreemapType.RESOURCES,
             hash="hash1",
@@ -79,7 +75,7 @@ def test_generate_with_small_localized_strings(self):
         localized_file_2 = FileInfo(
             full_path=Path("es.lproj/Localizable.strings"),
             path="es.lproj/Localizable.strings",
-            size=30 * 1024,  # 30KB
+            size=50 * 1024,  # 50KB * 0.5 = 25KB savings (total 55KB < 100KB threshold)
             file_type="strings",
             treemap_type=TreemapType.RESOURCES,
             hash="hash2",
@@ -97,14 +93,15 @@ def test_generate_with_small_localized_strings(self):
 
         result = self.insight.generate(insights_input)
 
-        assert result is None  # Should return None when below threshold
+        assert result is None  # Should return None when estimated savings below threshold
 
     def test_generate_with_exactly_threshold_size(self):
-        """Test that insight is generated when total size exactly equals 100KB threshold."""
+        """Test that insight is generated when estimated savings exceeds 100KB threshold."""
+        # Need 200KB to get exactly 100KB savings (200KB * 0.5 = 100KB)
         localized_file = FileInfo(
             full_path=Path("en.lproj/Localizable.strings"),
             path="en.lproj/Localizable.strings",
-            size=100 * 1024,  # Exactly 100KB
+            size=200 * 1024,  # 200KB * 0.5 = 100KB savings
             file_type="strings",
             treemap_type=TreemapType.RESOURCES,
             hash="hash1",
@@ -122,7 +119,7 @@ def test_generate_with_exactly_threshold_size(self):
 
         result = self.insight.generate(insights_input)
 
-        assert result is None  # Should return None when exactly at threshold
+        assert result is None  # Should return None when exactly at threshold (100KB, not >100KB)
 
     def test_generate_with_no_localized_strings(self):
         """Test that no insight is generated when no localized strings files exist."""
@@ -173,28 +170,37 @@ def test_generate_with_empty_file_list(self):
 
         assert result is None
 
-    def test_generate_ignores_non_localizable_strings(self):
-        """Test that only Localizable.strings files are considered, not other .strings files."""
-        localized_file = FileInfo(
+    def test_generate_includes_all_strings_files_except_denylisted(self):
+        """Test that all .strings files in .lproj directories are considered, except denylisted ones."""
+        localizable_file = FileInfo(
             full_path=Path("en.lproj/Localizable.strings"),
             path="en.lproj/Localizable.strings",
-            size=150 * 1024,  # 150KB - should trigger insight
+            size=150 * 1024,  # 150KB
             file_type="strings",
             treemap_type=TreemapType.RESOURCES,
             hash="hash1",
             is_dir=False,
         )
-        other_strings_file = FileInfo(
-            full_path=Path("en.lproj/Other.strings"),
-            path="en.lproj/Other.strings",
-            size=50 * 1024,  # 50KB - should be ignored
+        infoplist_file = FileInfo(
+            full_path=Path("en.lproj/InfoPlist.strings"),
+            path="en.lproj/InfoPlist.strings",
+            size=100 * 1024,  # 100KB - should be included
             file_type="strings",
             treemap_type=TreemapType.RESOURCES,
             hash="hash2",
             is_dir=False,
         )
+        launchscreen_file = FileInfo(
+            full_path=Path("en.lproj/LaunchScreen.strings"),
+            path="en.lproj/LaunchScreen.strings",
+            size=30 * 1024,  # 30KB - should be ignored (in denylist)
+            file_type="strings",
+            treemap_type=TreemapType.RESOURCES,
+            hash="hash3",
+            is_dir=False,
+        )
 
-        file_analysis = FileAnalysis(files=[localized_file, other_strings_file], directories=[])
+        file_analysis = FileAnalysis(files=[localizable_file, infoplist_file, launchscreen_file], directories=[])
 
         insights_input = InsightsInput(
             app_info=Mock(spec=BaseAppInfo),
@@ -206,17 +212,17 @@ def test_generate_ignores_non_localizable_strings(self):
         result = self.insight.generate(insights_input)
 
         assert isinstance(result, LocalizedStringInsightResult)
-        assert len(result.files) == 1
-        assert result.files[0].file_path == "en.lproj/Localizable.strings"
-        assert result.total_savings == 150 * 1024  # Only the Localizable.strings file
-        assert result.files[0].total_savings == 150 * 1024
+        # Should include localizable and infoplist, but not launchscreen
+        # Total size: (150KB + 100KB) * 0.5 = 125KB savings
+        expected_savings = int((150 + 100) * 1024 * 0.5)
+        assert result.total_savings == expected_savings
 
     def test_generate_ignores_non_lproj_localizable_strings(self):
         """Test that Localizable.strings files outside .lproj directories are ignored."""
         valid_localized_file = FileInfo(
             full_path=Path("en.lproj/Localizable.strings"),
             path="en.lproj/Localizable.strings",
-            size=150 * 1024,  # 150KB - should be included
+            size=250 * 1024,  # 250KB - should be included
             file_type="strings",
             treemap_type=TreemapType.RESOURCES,
             hash="hash1",
@@ -244,7 +250,88 @@ def test_generate_ignores_non_lproj_localizable_strings(self):
         result = self.insight.generate(insights_input)
 
         assert isinstance(result, LocalizedStringInsightResult)
-        assert len(result.files) == 1
-        assert result.files[0].file_path == "en.lproj/Localizable.strings"
-        assert result.total_savings == 150 * 1024  # Only the valid file
-        assert result.files[0].total_savings == 150 * 1024
+        # Only the valid file should be included: 250KB * 0.5 = 125KB savings
+        expected_savings = int(250 * 1024 * 0.5)
+        assert result.total_savings == expected_savings
+
+    def test_regex_pattern_matching(self):
+        """Test that the regex pattern correctly matches .lproj/.strings files."""
+        # Valid patterns that should match
+        valid_files = [
+            FileInfo(
+                full_path=Path("en.lproj/Localizable.strings"),
+                path="en.lproj/Localizable.strings",
+                size=250 * 1024,  # Large enough to trigger insight after 0.5 ratio
+                file_type="strings",
+                treemap_type=TreemapType.RESOURCES,
+                hash="hash1",
+                is_dir=False,
+            ),
+            FileInfo(
+                full_path=Path("Base.lproj/InfoPlist.strings"),
+                path="Base.lproj/InfoPlist.strings",
+                size=30 * 1024,
+                file_type="strings",
+                treemap_type=TreemapType.RESOURCES,
+                hash="hash2",
+                is_dir=False,
+            ),
+            FileInfo(
+                full_path=Path("pt-BR.lproj/Custom.strings"),
+                path="pt-BR.lproj/Custom.strings",
+                size=20 * 1024,
+                file_type="strings",
+                treemap_type=TreemapType.RESOURCES,
+                hash="hash3",
+                is_dir=False,
+            ),
+            FileInfo(
+                full_path=Path("some/path/en.lproj/file.strings"),  # Valid: frameworks can have .lproj dirs
+                path="some/path/en.lproj/file.strings",
+                size=50 * 1024,
+                file_type="strings",
+                treemap_type=TreemapType.RESOURCES,
+                hash="hash5",
+                is_dir=False,
+            ),
+        ]
+
+        # Invalid patterns that should NOT match
+        invalid_files = [
+            FileInfo(
+                full_path=Path("Localizable.strings"),  # Not in .lproj
+                path="Localizable.strings",
+                size=50 * 1024,
+                file_type="strings",
+                treemap_type=TreemapType.RESOURCES,
+                hash="hash4",
+                is_dir=False,
+            ),
+            FileInfo(
+                full_path=Path("en.lproj/subdir/file.strings"),  # Has subdirectory in .lproj
+                path="en.lproj/subdir/file.strings",
+                size=50 * 1024,
+                file_type="strings",
+                treemap_type=TreemapType.RESOURCES,
+                hash="hash6",
+                is_dir=False,
+            ),
+        ]
+
+        all_files = valid_files + invalid_files
+        file_analysis = FileAnalysis(files=all_files, directories=[])
+
+        insights_input = InsightsInput(
+            app_info=Mock(spec=BaseAppInfo),
+            file_analysis=file_analysis,
+            treemap=Mock(),
+            binary_analysis=[],
+        )
+
+        result = self.insight.generate(insights_input)
+
+        assert isinstance(result, LocalizedStringInsightResult)
+        # Should include valid files: (250 + 30 + 20 + 50) * 1024 * 0.5 = 175KB
+        # Note: some/path/en.lproj/file.strings is valid (frameworks can have .lproj dirs)
+        expected_savings = int((250 + 30 + 20 + 50) * 1024 * 0.5)
+        assert result.total_savings == expected_savings