Fix issue that strings with parameters are not parsed automatically

[Jack251970]

CHANGES

When comment of one localization string is empty, its parameters will be empty. So we should return the original parameters parameters.ToImmutableArray() instead of _emptyLocalizableStringParams.

TEST

We have one localization strings with parameters:

<system:String x:Key="flowlauncher_plugin_localization_demo_plugin_unused2">This is an unused string with parameters {2:00}, {1,-35:D} and {0}!!</system:String>

Originally, the result is:

Now, the result is:

[Copilot]

Pull Request Overview

This PR fixes a bug where localization strings with parameters would lose their parameters when comments are empty or invalid XML. The fix ensures that original parameters are preserved instead of being replaced with empty parameters.

Key changes:

• Remove unused _emptyLocalizableStringParams static field
• Return original parameters when comment is null/empty or XML parsing fails
• Add explanatory comments for the parameter preservation logic

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Updates LocalizeSourceGenerator.cs to remove a shared empty parameter array and adjust error/invalid-comment handling to return the provided parameters as an immutable array. The change affects parameter parsing flow inside ParseCommentAndUpdateParameters, with no public API modifications.

Changes

Cohort / File(s)	Change summary
Localization Source Generator Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs	Removed _emptyLocalizableStringParams. In ParseCommentAndUpdateParameters, on null/invalid comment or exceptions, now returns (null, parameters.ToImmutableArray()) instead of (null, _emptyLocalizableStringParams). Adjusted usages accordingly. No exported/public signatures changed.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Compiler
  participant LSG as LocalizeSourceGenerator
  participant Parser as ParseCommentAndUpdateParameters

  Compiler->>LSG: Execute()
  LSG->>Parser: Parse(comment, parameters)
  alt Valid comment
    Parser-->>LSG: (parsedText, updatedParams)
  else Null/invalid comment or exception
    note over Parser: New behavior: return input parameters as immutable
    Parser-->>LSG: (null, parameters.ToImmutableArray())
  end
  LSG-->>Compiler: Generated sources (based on returned tuple)

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• Resolve namespace prefix dynamically #15: Also changes how empty/missing LocalizableString parameters are represented in LocalizeSourceGenerator.cs.
• Fix Index Issue for Format String & Add Culture Info When Need to Format #17: Modifies parameter parsing and return handling in ParseCommentAndUpdateParameters, similar area of code.

Suggested labels

bug

Suggested reviewers

• Yusyuriv

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title succinctly and accurately summarizes the primary change: fixing automatic parsing of parameterized localization strings. It directly matches the code modification in LocalizeSourceGenerator.cs that preserves parsed parameters instead of using a shared empty parameter array. The phrasing is concise and clear for a teammate scanning history.
Description Check	✅ Passed	The description clearly explains the bug, the code change, and includes a concrete test case with before-and-after evidence, so it is directly related to the changeset and satisfies this lenient check. It documents the rationale and verification steps used to confirm the fix.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch auto_generate_variables

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

• Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
• Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs (3)

357-359: Preserve parsed params when comment is absent/invalid — correct fix; tighten guard + tiny alloc tweak

Returning parameters.ToImmutableArray() here fixes the bug and preserves parsed placeholders. Consider clarifying the comment and making the guard more precise (avoid Value == null, prefer whitespace check) and skip an allocation when parameters is empty.

Proposed diff:

-            if (comment == null || comment.Value == null || parameters.Count == 0)
+            if (parameters.Count == 0 || comment is null || string.IsNullOrWhiteSpace(comment.Value))
             {
-                // If the comment is not a valid XML, the summary is null and the parameters are not updated
-                return (null, parameters.ToImmutableArray());
+                // No comment (or empty) or no format params: pass through parsed parameters unchanged
+                return (null, parameters.Count == 0 
+                    ? ImmutableArray<LocalizableStringParam>.Empty 
+                    : parameters.ToImmutableArray());
             }

Please confirm you have a test covering:

• Comment missing
• Comment empty/whitespace
• Comment present but invalid XML

---

369-383: Avoid NullReference when attributes are missing; don’t rely on exceptions for control flow

Accessing .Value on a missing attribute can throw and fall into the catch path. Parse attributes defensively to keep the happy path exception‑free.

-                    var index = int.TryParse(p.Attribute(Constants.IndexAttribute).Value, out var intValue) ? intValue : -1;
-                    var name = p.Attribute(Constants.NameAttribute).Value;
-                    var type = p.Attribute(Constants.TypeAttribute).Value;
+                    var idxAttr = p.Attribute(Constants.IndexAttribute)?.Value;
+                    var name = p.Attribute(Constants.NameAttribute)?.Value;
+                    var type = p.Attribute(Constants.TypeAttribute)?.Value;
+                    var index = !string.IsNullOrEmpty(idxAttr) && int.TryParse(idxAttr, out var intValue)
+                        ? intValue
+                        : -1;

---

388-390: Exception path now preserves parsed params — good; optionally surface diagnostics

Behavior matches the intent (keep parsed params if comment parsing fails). If feasible later, consider reporting a diagnostic (at low severity) for malformed comment XML to aid authors.

If you want, I can add a follow-up PR to plumb a non-throwing diagnostic here (e.g., via a helper) so generator users see why summaries/param metadata weren’t applied.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d022ba5 and 7fdb0ac.

📒 Files selected for processing (1)

• Flow.Launcher.Localization.SourceGenerators/Localize/LocalizeSourceGenerator.cs (2 hunks)

⏰ 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: build