[Feature] Linked Files #770
Conversation
Getting file link to all key not just first level Getting file link in data from files datas
WalkthroughIntroduces linked translation file resolution in RootBundleAssetLoader with depth limit and cycle detection, updates README with a “Linked files” section (duplicated), adjusts test timing, adds comprehensive tests for linked files, and adds/updates i18n JSON assets to exercise single, multiple, nested, deep, cyclic, and missing link scenarios. en.json now links hats externally. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant Controller as EasyLocalizationController
participant Loader as RootBundleAssetLoader
participant Bundle as rootBundle
App->>Controller: loadTranslations()
Controller->>Loader: load(path, locale)
Loader->>Bundle: loadString(base locale JSON)
Bundle-->>Loader: baseJson (Map)
rect rgb(242,248,255)
note right of Loader: Resolve linked values recursively<br/>with depth limit and cycle detection
loop For each entry in JSON
alt value startsWith ":/"
Loader->>Loader: _getLinkedLocalePath()
Loader->>Bundle: loadString(linked file for locale)
Bundle-->>Loader: linkedJson (Map)
Loader->>Loader: recurse(depth+1, visited)
Loader-->>Loader: resolved Map
Loader-->>Loader: replace string with resolved Map
else value is Map
Loader->>Loader: recurse into nested Map
else
note right of Loader: keep primitive as-is
end
end
end
Loader-->>Controller: expanded translations (Map)
Controller-->>App: ready
alt Cycle detected or depth exceeded
Loader-->>Controller: throw StateError
Controller-->>App: error surfaced (FlutterError)
else Missing file
Loader-->>Controller: throw StateError
Controller-->>App: error surfaced (FlutterError)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (15)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (11)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ 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)
🔇 Additional comments (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
README.md (4)
412-416: Fix heading punctuation and wording; align terminology with JSON
- Remove trailing colon from the heading (MD026)
- Prefer “JSON files” and clearer phrasing in the note.
-### 🔥 Linked files: +### 🔥 Linked files @@ -> ⚠ This is only available for the default asset loader (on Json Files). +> ⚠ This is only available for the default asset loader (JSON files only).
429-437: Specify a language for the fenced code blockmarkdownlint (MD040) flags this block. Use a neutral language like text.
-``` +```text assets └── translations └── en-US ├── errors.json ├── validation.json └── notifications.json--- `440-440`: **Tighten grammar and fix the anchor link** - Remove the extra space before the colon. - The anchor for “Installation” should be #installation (emoji is ignored in GitHub anchors). ```diff -Don't forget to add your linked files (or linked files folder, here assets/translations/en-US/), to your pubspec.yaml : [See installation](#-installation). +Don't forget to add your linked files (or the folder assets/translations/en-US/) to your pubspec.yaml. See [Installation](#installation).
416-441: Optional: Add a minimal pubspec example for linked subfoldersReaders may wonder whether subfolders must be declared explicitly. Consider appending a short snippet:
flutter: assets: - assets/translations/ # includes subfolders like en-US/, de-DE/, etc. # or, if you prefer being explicit: # - assets/translations/en-US/lib/src/asset_loader.dart (1)
33-35: Nit: normalize leading slashes in filePath at the join siteWhile we sanitize in the recursive loader, it’s safer if this join also tolerates leading slashes in filePath.
- String _getLinkedLocalePath(String basePath, String filePath, Locale locale) { - return '$basePath/${locale.toStringWithSeparator(separator: "-")}/$filePath'; - } + String _getLinkedLocalePath(String basePath, String filePath, Locale locale) { + final sanitized = filePath.replaceAll(RegExp(r'^[\\/]+'), ''); + return '$basePath/${locale.toStringWithSeparator(separator: "-")}/$sanitized'; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
README.md(2 hunks)lib/src/asset_loader.dart(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~412-~412: There might be a mistake here.
Context: ...your full name ``` ### 🔥 Linked files: > ⚠ This is only available for the default ...
(QB_NEW_EN)
[grammar] ~440-~440: There might be a mistake here.
Context: ...s/translations/en-US/), to your pubspec.yaml : See installation. ##...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
README.md
412-412: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
429-429: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
Was needed for test to pass, everything is working as intended. iddle is probably need be cause de loading is slightly longer
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/src/asset_loader.dart (1)
69-72: Improve logging clarity and seed traversal state with typed decode
- Log the actual asset path being loaded, not just base path.
- Cast json.decode to Map<String, dynamic> and pass initial visited/cache to the helper.
Apply this diff:
- EasyLocalization.logger.debug('Load asset from $path'); - - Map<String, dynamic> baseJson = json.decode(await rootBundle.loadString(localePath)); - return await _getLinkedTranslationFileDataFromBaseJson(path, locale, baseJson); + EasyLocalization.logger.debug('Load asset from $localePath'); + + final Map<String, dynamic> baseJson = + json.decode(await rootBundle.loadString(localePath)) as Map<String, dynamic>; + return await _getLinkedTranslationFileDataFromBaseJson( + path, + locale, + baseJson, + visited: <String>{}, + cache: <String, Map<String, dynamic>>{}, + );
🧹 Nitpick comments (5)
test/easy_localization_context_test.dart (1)
182-187: Stabilize after resetLocale with pumpAndSettleAfter calling _context.resetLocale() (Line 184), a plain pump() (Line 185) may not always await all async/UI work. Prefer pumpAndSettle() here for stability.
Apply this diff:
- await _context.resetLocale(); - await tester.pump(); + await _context.resetLocale(); + await tester.pumpAndSettle();test/easy_localization_widget_test.dart (3)
91-99: Good call enabling tester.idle(); also modernize matcher to isA()The idle() addition is appropriate. Minor nit: isInstanceOf() is deprecated; use isA() for consistency (you already use it elsewhere in this file).
Apply this diff:
- expect(Localization.of(_context), isInstanceOf<Localization>()); - expect(Localization.instance, isInstanceOf<Localization>()); - expect(Localization.instance, Localization.of(_context)); + expect(Localization.of(_context), isA<Localization>()); + expect(Localization.instance, isA<Localization>()); + expect(Localization.instance, Localization.of(_context));
183-197: Fix test name typo: “loacle” → “locale”Typo in test description. Keeps test outputs clean.
Apply this diff:
- '[EasyLocalization] change loacle test', + '[EasyLocalization] change locale test',
262-268: Avoid set literal in async closure passed to expectUsing () async => { await ... } creates a Set literal, which is confusing. Use a block closure or return the Future expression directly.
Apply this diff:
- l = const Locale('en', 'UK'); - expect(() async => {await EasyLocalization.of(_context)!.setLocale(l)}, throwsAssertionError); + l = const Locale('en', 'UK'); + expect(() async { + await EasyLocalization.of(_context)!.setLocale(l); + }, throwsAssertionError);lib/src/asset_loader.dart (1)
33-35: Consider normalizing/sanitizing linked file paths_getLinkedLocalePath assumes filePath is safe. If callers supply values like '/x' or '../x', rootBundle may still resolve declared assets unexpectedly. The refactor above sanitizes before calling this function; keep that contract documented or add checks here as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
lib/src/asset_loader.dart(1 hunks)test/easy_localization_context_test.dart(7 hunks)test/easy_localization_widget_test.dart(26 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 (4)
test/easy_localization_context_test.dart (2)
141-141: Enabling tester.idle() improves async determinism — LGTMActivating await tester.idle() after pumpWidget/pump helps settle microtasks before the next frame. This should reduce flakiness around async delegators.
Also applies to: 159-159, 171-171, 200-200, 219-219
91-91: Inline supportedLocales arrays — stylistic change is fineConsolidating multi-line arrays to single-line is consistent and keeps tests concise. No behavioral change.
Also applies to: 118-118, 139-139, 156-156, 174-174, 197-197, 215-215
test/easy_localization_widget_test.dart (2)
667-672: Using pumpAndSettle for device/locale flows — LGTMThe switch to pumpAndSettle makes these flows less flaky by waiting for all frames to settle.
Also applies to: 690-695
783-784: Assertion on plural text after locale change — LGTMVerification after setLocale + pumpAndSettle looks correct.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/src/asset_loader.dart (1)
110-110: Explicitly cast decoded JSON to Map<String, dynamic> (avoid implicit cast at runtime)This was called out earlier; add an explicit cast for safety and clearer intent.
Apply this diff:
- Map<String, dynamic> baseJson = json.decode(await rootBundle.loadString(localePath)); + final Map<String, dynamic> baseJson = + json.decode(await rootBundle.loadString(localePath)) as Map<String, dynamic>;
🧹 Nitpick comments (4)
lib/src/asset_loader.dart (4)
27-29: Remove leftover placeholder commentStray scaffolding comment in source; keep only the constant.
Apply this diff:
- // Place inside class RootBundleAssetLoader static const int _maxLinkedDepth = 32;
60-65: Harden traversal check; avoid false positives on filenames containing “..”normalizedPath.contains('..') rejects legitimate names like "foo..json". Check path segments for '..' instead and guard against empty paths.
Apply this diff:
- // Normalize and reject traversal - final normalizedPath = rawPath.replaceAll(RegExp(r'^[\\/]+'), ''); - if (normalizedPath.contains('..')) { - throw FormatException('Invalid linked file path "$rawPath" for key "$key".'); - } + // Normalize leading separators and reject traversal via ".." segments + final normalizedPath = rawPath.replaceAll(RegExp(r'^[\\/]+'), ''); + if (normalizedPath.isEmpty) { + throw FormatException('Invalid linked file path "$rawPath" for key "$key".'); + } + final parts = normalizedPath.split(RegExp(r'[\\/]+')); + if (parts.any((p) => p == '..')) { + throw FormatException('Invalid linked file path "$rawPath" for key "$key".'); + }
40-103: Arrays aren’t traversed; linked strings inside lists won’t be resolvedCurrent traversal handles maps and direct string links but skips lists. If translations may contain arrays (e.g., bullet points) with links or nested maps, they won’t be expanded.
- If arrays are out of scope, clarify in README that linking applies to object values only.
- If needed, I can add safe List traversal that mirrors the Map logic (cycle checks, caching). Want me to draft that?
105-118: Add tests for cycles, depth, caching, and sanitizationThe implementation is solid; let’s lock behavior in with tests (happy path and failure cases).
Suggested cases:
- Link expansion across multiple files and nested folders.
- Cycle detection (A → B → A) and max-depth exceeded.
- Path sanitization rejects "../x.json" and accepts "foo..json".
- Cache effectiveness: same linked file included multiple times should load once.
I can scaffold widget/loader tests using TestAssetBundle to assert load counts and thrown errors. Want me to open a follow-up PR with these tests?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/src/asset_loader.dart(1 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
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
lib/src/asset_loader.dart (3)
65-67: Add a per-load cache to dedupe asset readsThe same linked file can be referenced multiple times. Cache parsed JSON per asset path to avoid repeated IO/decoding.
- Future<Map<String, dynamic>> _getLinkedTranslationFileDataFromBaseJson( + Future<Map<String, dynamic>> _getLinkedTranslationFileDataFromBaseJson( String basePath, Locale locale, Map<String, dynamic> baseJson, { required Set<String> visited, int depth = 0, }) async { @@ - final Map<String, dynamic> linkedJson = - json.decode(await rootBundle.loadString(linkedAssetPath)) as Map<String, dynamic>; + final Map<String, dynamic> linkedJson = + (cache[linkedAssetPath] ??= (json.decode( + await rootBundle.loadString(linkedAssetPath), + ) as Map<String, dynamic>)); @@ - final resolved = await _getLinkedTranslationFileDataFromBaseJson( + final resolved = await _getLinkedTranslationFileDataFromBaseJson( basePath, locale, linkedJson, - visited: visited, + visited: visited, + cache: cache, depth: depth + 1, );And update the function signature accordingly:
- Map<String, dynamic> baseJson, { - required Set<String> visited, - int depth = 0, - }) async { + Map<String, dynamic> baseJson, { + required Set<String> visited, + required Map<String, Map<String, dynamic>> cache, + int depth = 0, + }) async {Note: See the load(...) comment below for seeding the cache at the call site.
Also applies to: 70-76
94-100: Cast decoded JSON and seed traversal state (optionally add cache)
- Strongly type the decoded base JSON.
- If you adopt caching (suggested above), pass an empty cache here.
Minimal typing fix:
- Map<String, dynamic> baseJson = json.decode(await rootBundle.loadString(localePath)); + final Map<String, dynamic> baseJson = + json.decode(await rootBundle.loadString(localePath)) as Map<String, dynamic>;If you implement caching, also pass it:
- return await _getLinkedTranslationFileDataFromBaseJson( + return await _getLinkedTranslationFileDataFromBaseJson( path, locale, baseJson, - visited: <String>{}, + visited: <String>{}, + cache: <String, Map<String, dynamic>>{}, );
57-60: Sanitize linked paths: strip leading slashes and reject parent traversalRaw paths like "/x.json" or "../x.json" should not be allowed. Normalize the link and reject ".." to prevent escaping the locale directory.
- final rawPath = value.substring(2).trim(); - final linkedAssetPath = _getLinkedLocalePath(basePath, rawPath, locale); + final rawPath = value.substring(2).trim(); + // Strip leading slashes/backslashes and reject parent traversal + final normalizedPath = rawPath.replaceAll(RegExp(r'^[\\/]+'), ''); + if (normalizedPath.contains('..')) { + throw FormatException('Invalid linked file path "$rawPath" for key "$key".'); + } + final linkedAssetPath = _getLinkedLocalePath(basePath, normalizedPath, locale);
🧹 Nitpick comments (2)
lib/src/asset_loader.dart (2)
27-29: Remove leftover guidance comment; keep the constantThe inline note "Place inside class RootBundleAssetLoader" looks like a migration hint and should be removed.
- // Place inside class RootBundleAssetLoader static const int _maxLinkedDepth = 32;
78-82: Preserve stack traces when rethrowingWrap with Error.throwWithStackTrace to retain the original stack for easier debugging.
- } catch (e) { - throw StateError( - 'Error resolving linked file "$linkedAssetPath" for key "$key": $e', - ); - } + } catch (e, st) { + Error.throwWithStackTrace( + StateError('Error resolving linked file "$linkedAssetPath" for key "$key": $e'), + st, + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/src/asset_loader.dart(1 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 (1)
lib/src/asset_loader.dart (1)
36-38: LGTM: locale-scoped path helperThe helper composes the locale-scoped asset path consistently with the main getLocalePath.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/src/asset_loader.dart (1)
57-60: Sanitize link targets: normalize and reject traversal/absolute pathsPrevent accidental “..” traversal or absolute paths by normalizing the target before composing the asset path. This also improves error messages.
Apply this diff:
- if (value is String && value.startsWith(':/')) { - final rawPath = value.substring(2).trim(); - final linkedAssetPath = _getLinkedLocalePath(basePath, rawPath, locale); + if (value is String && value.startsWith(':/')) { + final rawPath = value.substring(2).trim(); + // Strip any leading slashes and reject parent traversal. + final normalizedPath = rawPath.replaceAll(RegExp(r'^[\\/]+'), ''); + if (normalizedPath.contains('..')) { + throw FormatException('Invalid linked file path "$rawPath" for key "$key".'); + } + final linkedAssetPath = _getLinkedLocalePath(basePath, normalizedPath, locale);
🧹 Nitpick comments (7)
lib/src/asset_loader.dart (7)
27-29: Remove leftover prompt comment; keep or document the constantThe inline “Place inside class …” comment looks like a prompt stub and should be removed. Consider adding a brief doc comment for clarity.
Apply this diff:
- // Place inside class RootBundleAssetLoader - static const int _maxLinkedDepth = 32; + /// Maximum allowed include depth to avoid runaway recursion. + static const int _maxLinkedDepth = 32;
27-29: Optional: make the depth ceiling configurable (without breaking const constructor)If you foresee projects needing a different ceiling, expose it as an optional parameter with a const default so existing usage and const constructor remain valid.
Example (outside selected lines):
// Inside class: final int maxLinkedDepth; const RootBundleAssetLoader({this.maxLinkedDepth = _maxLinkedDepth}); // Use `maxLinkedDepth` instead of `_maxLinkedDepth` in the guard.
36-38: Nit: path joiningString concatenation works, but it’s brittle if callers accidentally pass leading/trailing slashes. You already normalize later; alternatively consider joining with a posix join helper or stripping leading slashes here as well.
61-63: Clarify the error message: duplicates are disallowed by design, not just cyclesPer the feature’s contract, a second include of the same file is forbidden even if it’s not a cycle. Update the message to reflect both cases.
Apply this diff:
- if (visited.contains(linkedAssetPath)) { - throw StateError('Cyclic linked files detected at "$linkedAssetPath" (key: "$key").'); - } + if (visited.contains(linkedAssetPath)) { + throw StateError( + 'Linked file reuse or cycle detected at "$linkedAssetPath" (key: "$key"). ' + 'Reusing linked files is not allowed.', + ); + }
77-84: Make control flow explicit and preserve stack traces on error
- Add a
continue;after assigning the resolved map to avoid falling through to the subsequent Map-branch check.- Preserve the original stack trace when rethrowing.
Apply this diff:
); fullJson[key] = resolved; - } catch (e) { - throw StateError( - 'Error resolving linked file "$linkedAssetPath" for key "$key": $e', - ); - } + continue; + } catch (e, st) { + Error.throwWithStackTrace( + StateError('Error resolving linked file "$linkedAssetPath" for key "$key": $e'), + st, + ); + }
85-93: Optional: recurse into lists if you decide to support themCurrently only nested Map values are traversed. If list-typed values can contain link markers or nested maps in your schema, you’ll need to handle List as well.
104-110: Cast the decoded base JSON to a typed mapAlign with the linked-file decode and make the type explicit.
Apply this diff:
- Map<String, dynamic> baseJson = json.decode(await rootBundle.loadString(localePath)); + final Map<String, dynamic> baseJson = + json.decode(await rootBundle.loadString(localePath)) as Map<String, dynamic>;Optional: log the resolved asset path to aid debugging (outside selected lines):
EasyLocalization.logger.debug('Load asset from $localePath');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/src/asset_loader.dart(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T11:49:13.323Z
Learnt from: JavedNicolas
PR: aissat/easy_localization#770
File: lib/src/asset_loader.dart:61-63
Timestamp: 2025-08-19T11:49:13.323Z
Learning: In the easy_localization linked files feature, files must not be used multiple times during translation resolution. The visited set in RootBundleAssetLoader._getLinkedTranslationFileDataFromBaseJson is intentionally never cleared to enforce this constraint - once a linked file is processed, it cannot be reused elsewhere in the same translation tree, even in different branches.
Applied to files:
lib/src/asset_loader.dart
⏰ 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/asset_loader.dart (3)
40-49: Depth guard is good defensive programmingThe early return on excessive depth protects against runaway recursion. Looks good.
65-66: Good: typed cast for decoded JSONCasting the decoded linked JSON to Map<String, dynamic> avoids type ambiguity at runtime.
68-76: Acknowledged: not clearingvisitedis intentional policyLeaving entries in
visitedto disallow reuse across branches aligns with the stated design. No change requested.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
pubspec.yaml (1)
28-31: Consider test-only asset loading to avoid shipping fixtures to consumersDeclaring i18n/ as package assets will bundle these test fixtures into every consuming app, increasing their bundle size unnecessarily. If these assets are only used by tests:
- Option A (preferred): Switch tests to a custom/Test AssetBundle that reads from test fixtures on disk, and remove the assets declaration.
- Option B: Move fixtures under an example app and declare them there.
If tests depend on rootBundle with these assets, keep as-is; otherwise, consider the refactor above.
Would you like help drafting a minimal TestAssetBundle helper and updating the tests to avoid including these assets in the published package?
i18n/en.json (1)
11-11: LGTM: switched plural "hats" to a linked file referenceThe link syntax and placement look correct for locale-scoped resolution (:/hats.json → i18n/en/hats.json). Minor note: "hat_other.other" duplicates the "other hats" text present under hats.other now; keep if intentionally supporting an existing key.
test/asset_loader_linked_files_test.dart (6)
25-27: Propagate loader errors in tests to avoid false positivesIn positive-path tests you currently log errors. If a load error happens, the test may proceed and fail later with a less-informative assertion. Consider immediately failing in the error callback.
Example change per occurrence:
- onLoadError: (FlutterError e) { - log(e.toString()); - }, + onLoadError: (FlutterError e) { + fail('Unexpected load error: $e'); + },You’re already rethrowing in the negative-path tests, which is good.
Also applies to: 49-51, 76-78, 100-101, 123-124, 154-156, 178-180, 204-205
63-66: Clarify misleading comment: these are different files, not multiple references to the same fileThe code checks values from validation.json and multi_validation.json (and errors.json vs multi_errors.json). It’s not verifying reuse of the same file.
- // Check multiple references to same file work + // Check multiple linked files from different sources are loaded
139-141: Prefer matchers for type assertionsUse matchers to improve failure messages and readability.
- expect(result['app']['errors'] is Map, true); - expect(result['app']['errors'] is String, false); + expect(result['app']['errors'], isA<Map>()); + expect(result['app']['errors'], isNot(isA<String>()));
160-166: Correct the failure message: the test expects FlutterError, not StateErrorThe test asserts FlutterError, so the fail message should reflect that.
- fail('Expected StateError to be thrown'); + fail('Expected FlutterError to be thrown');
15-16: Optional: factor controller creation to reduce duplication and centralize the asset pathA small helper will DRY up repeated setup and ensure the asset path stays consistent:
// Add near the top (after imports) const _kI18nAssetsPath = 'i18n'; EasyLocalizationController _buildController({ required Locale locale, bool useOnlyLangCode = false, void Function(FlutterError e)? onLoadError, }) { return EasyLocalizationController( forceLocale: locale, path: _kI18nAssetsPath, supportedLocales: [locale], useOnlyLangCode: useOnlyLangCode, useFallbackTranslations: false, saveLocale: false, onLoadError: onLoadError ?? (e) => fail('Unexpected load error: $e'), assetLoader: const RootBundleAssetLoader(), ); }Then replace repeated controller initializations with _buildController(locale: const Locale('en', 'linked')) etc.
144-167: Consider adding a test for "no reuse of the same linked file across branches" (by design)Per feature design, once a linked file is processed, it must not be reused elsewhere in the same resolution tree (visited set is not cleared). Add a negative-path test to lock this behavior.
Example sketch:
test('should error when the same linked file is referenced in two branches', () async { final controller = _buildController( locale: const Locale('en', 'linked'), onLoadError: (e) => throw e, ); // Create a fixture JSON (e.g., i18n/en-dup.json) where two different branches // both point to :/shared.json to trigger the "already visited" rule. controller.forceLocale = const Locale('en', 'dup'); try { await controller.loadTranslations(); fail('Expected FlutterError to be thrown for duplicate linked file usage'); } catch (e) { expect(e, isA<FlutterError>()); expect(e.toString(), contains('already processed')); // or the specific message you emit } });If you’d like, I can draft the dup fixtures and test case in a follow-up commit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
i18n/en-cyclic.json(1 hunks)i18n/en-cyclic/cycle_file1.json(1 hunks)i18n/en-cyclic/cycle_file2.json(1 hunks)i18n/en-linked.json(1 hunks)i18n/en-linked/deep/level1.json(1 hunks)i18n/en-linked/deep/level2.json(1 hunks)i18n/en-linked/errors.json(1 hunks)i18n/en-linked/multi_errors.json(1 hunks)i18n/en-linked/multi_validation.json(1 hunks)i18n/en-linked/nested/messages.json(1 hunks)i18n/en-linked/validation.json(1 hunks)i18n/en-missing.json(1 hunks)i18n/en.json(1 hunks)i18n/en/hats.json(1 hunks)pubspec.yaml(1 hunks)test/asset_loader_linked_files_test.dart(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- i18n/en-missing.json
- i18n/en-linked/deep/level2.json
- i18n/en-cyclic/cycle_file1.json
- i18n/en-linked/errors.json
- i18n/en-linked/multi_errors.json
- i18n/en/hats.json
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: JavedNicolas
PR: aissat/easy_localization#770
File: lib/src/asset_loader.dart:61-63
Timestamp: 2025-08-19T11:49:13.323Z
Learning: In the easy_localization linked files feature, files must not be used multiple times during translation resolution. The visited set in RootBundleAssetLoader._getLinkedTranslationFileDataFromBaseJson is intentionally never cleared to enforce this constraint - once a linked file is processed, it cannot be reused elsewhere in the same translation tree, even in different branches.
📚 Learning: 2025-08-19T11:49:13.323Z
Learnt from: JavedNicolas
PR: aissat/easy_localization#770
File: lib/src/asset_loader.dart:61-63
Timestamp: 2025-08-19T11:49:13.323Z
Learning: In the easy_localization linked files feature, files must not be used multiple times during translation resolution. The visited set in RootBundleAssetLoader._getLinkedTranslationFileDataFromBaseJson is intentionally never cleared to enforce this constraint - once a linked file is processed, it cannot be reused elsewhere in the same translation tree, even in different branches.
Applied to files:
test/asset_loader_linked_files_test.dart
🔇 Additional comments (7)
i18n/en-linked/nested/messages.json (1)
1-5: LGTM: valid JSON and paths look correct for nested linkingKeys/values are well-formed. The link target implied by en-linked.json (:/nested/messages.json) matches this file’s location. No further changes needed.
i18n/en-cyclic.json (1)
1-4: Intentionally cyclic fixture: good for exercising cycle detectionThis looks purpose-built for the cycle tests and aligns with the resolver behavior. No issues spotted with structure or paths.
i18n/en-linked/deep/level1.json (1)
1-4: LGTM: deep link target path matches expected structureThe link “:/deep/level2.json” correctly maps relative to en-linked/, and the file format is valid.
i18n/en-linked/validation.json (1)
1-6: LGTM: placeholders and key naming are consistentPlaceholders {min}/{max} align with easy_localization’s named replacements. One note based on the linked-files design: the visited-set constraint means the same linked file cannot be reused in multiple branches of a single resolution. Ensure this file isn’t linked from multiple branches in the same tree if that would conflict with the intended behavior.
If there’s a scenario requiring reuse of the same validation file in multiple branches, we can suggest a structure that deduplicates content or uses small split files to avoid the visited-set restriction. Want a proposal?
i18n/en-linked.json (1)
1-19: LGTM: comprehensive linked structure for sample datasetAll referenced linked paths are coherent and align with the test coverage (single, multiple, nested, deep).
i18n/en-linked/multi_validation.json (1)
1-6: LGTM: validation messages for "multiple" groupContent aligns with tests. Placeholders {min}/{max} preserved.
i18n/en-cyclic/cycle_file2.json (1)
1-4: LGTM: cycle established for error-path testingThe back-link to cycle_file1.json correctly forms the cycle used by tests.
Adds tests for linked files, including error handling for cyclic dependencies and missing files. Includes a fix to consider the `useOnlyLangCode` flag.
Allow user to split the translations files to keep translations maintanable.
PS : Only for default Asset loader and JSON format. Might need some more work for other type of files and custom asset loaders
Summary by CodeRabbit