-
-
Notifications
You must be signed in to change notification settings - Fork 367
feat: Added recursivity to replace link to catch nested links #783
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this 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 referencesThe recursive
_replaceLinksimplementation 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,_replaceLinkskeeps 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 = 50and 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: AvoidRegExp('{$key}')here; a plain string pattern is safer and cheaperUsing
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
📒 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_linkKeyMatcherlooks correct for supporting parenthesized link keysThe new pattern allowing either
@:foo.baror@:(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: Staticofshortcut refactor is fineSwitching
ofto an expression-bodied method aroundLocalizations.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 currentLocalizationsDelegatesetup.
42-42: Confirm intended behavior ofuseFallbackTranslationsForEmptyResourcesfor both main and fallback localesThe new checks:
if (resource == null || (_useFallbackTranslationsForEmptyResources && resource.isEmpty)) { ... resource = _fallbackTranslations?.get(key); if (resource == null || (_useFallbackTranslationsForEmptyResources && resource.isEmpty)) { ... } }mean that when
useFallbackTranslationsForEmptyResourcesis 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
Enable the possibility to get nested links
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.