Skip to content

Conversation

@JavedNicolas
Copy link
Contributor

@JavedNicolas JavedNicolas commented Dec 15, 2025

Enable the possibility to get nested links

Summary by CodeRabbit

  • Bug Fixes
    • Improved localization link resolution to properly handle nested link placeholders. All link references within localization strings are now fully resolved recursively.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 15, 2025

Walkthrough

The Localization._replaceLinks method is enhanced with recursive link replacement to handle nested link patterns. After initial single-pass replacements, the method now invokes itself when results still contain link patterns, ensuring complete resolution of all link placeholders. Formatting adjustments are also applied throughout.

Changes

Cohort / File(s) Summary
Internal Link Resolution and Formatting
lib/src/localization.dart
Added recursive invocation of _replaceLinks to resolve nested link patterns. Applied formatting adjustments for code consistency. No changes to public APIs.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Focus on the recursive link replacement logic to ensure termination conditions are safe and prevent infinite loops
  • Verify that nested link resolution produces expected output and doesn't alter behavior for non-nested cases
  • Check formatting changes maintain consistency with project style guidelines

Poem

🐰 Hop, skip, and jump through links so deep,
Nested treasures we now seek,
Recursion's dance, round and round,
Every placeholder shall be found! 🔗✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding recursion to the link replacement mechanism to handle nested links, which directly matches the core modification in lib/src/localization.dart.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
lib/src/localization.dart (1)

68-103: Add depth guard to prevent infinite recursion on circular link references

The recursive _replaceLinks implementation introduces a critical stack overflow risk when translation keys reference themselves (e.g., "foo": "@:foo") or form cycles (e.g., "foo": "@:bar" and "bar": "@:foo"). In both cases, _replaceLinks keeps recursing because the result remains unchanged by the replacement but _linkKeyMatcher.hasMatch(result) stays true, eventually causing a stack overflow.

Replace the explicit recursion with a bounded loop as shown in your proposed diff. The approach with maxIterations = 50 and logged warning when depth is exceeded is sound.

Additionally, add tests covering:

  • A long acyclic chain of links that fully resolves
  • Self-link and 2-cycle cases that gracefully handle the depth limit without crashing
🧹 Nitpick comments (1)
lib/src/localization.dart (1)

113-117: Avoid RegExp('{$key}') here; a plain string pattern is safer and cheaper

Using RegExp('{$key}') for named args:

args.forEach((String key, String value) => res = res.replaceAll(RegExp('{$key}'), value));

is overkill and subtly changes semantics if keys ever contain regex metacharacters. A literal string pattern is enough and avoids surprises and per‑call regex compilation.

Consider reverting to a simple literal replacement:

-    args.forEach((String key, String value) => res = res.replaceAll(RegExp('{$key}'), value));
+    args.forEach((key, value) => res = res.replaceAll('{$key}', value));

Given the docs show keys like {lang}, {name}, this keeps behavior identical for existing users while being more robust.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 25b7853 and a466ce8.

📒 Files selected for processing (1)
  • lib/src/localization.dart (7 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: Codacy Static Code Analysis
🔇 Additional comments (3)
lib/src/localization.dart (3)

12-15: Updated _linkKeyMatcher looks correct for supporting parenthesized link keys

The new pattern allowing either @:foo.bar or @:(foo.bar) aligns with the stripping done via _bracketsMatcher. Behavior looks good as long as tests cover both styles and mixed text around them.

Please ensure you have tests for:

  • @:example.hello @:example.world
  • @:(example.hello) embedded in longer strings.

30-30: Static of shortcut refactor is fine

Switching of to an expression-bodied method around Localizations.of<Localization> is a pure formatting change with no behavioral impact. Looks good.

Consider adding/keeping a small widget test to confirm Localization.of(context) continues to resolve correctly under the current LocalizationsDelegate setup.


42-42: Confirm intended behavior of useFallbackTranslationsForEmptyResources for both main and fallback locales

The new checks:

if (resource == null || (_useFallbackTranslationsForEmptyResources && resource.isEmpty)) {
  ...
  resource = _fallbackTranslations?.get(key);
  if (resource == null || (_useFallbackTranslationsForEmptyResources && resource.isEmpty)) {
    ...
  }
}

mean that when useFallbackTranslationsForEmptyResources is true:

  • Empty strings in the primary translations behave as “missing” and will try the fallback.
  • Empty strings in the fallback translations are also treated as “missing”, returning the key and logging a warning.

This is probably what you want (empty strings are considered configuration errors), but it is a behavior change compared to returning '' from the fallback locale. It’s worth double‑checking this against your documented semantics and adding tests that cover:

  • Empty value in main locale, non‑empty in fallback.
  • Empty in both main and fallback.

If you instead want to allow empty strings in fallback translations while still treating primary‑locale empties as missing, we can tweak the second condition accordingly.

Also applies to: 201-220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant