Remove old dictionaries references to fix possible memory leak #3990
Conversation
|
🥷 Code experts: no user but you matched threshold 10 Jack251970, jjw24 have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
|
Warning Rate limit exceeded@Jack251970 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughSerialized language-change flow by adding a semaphore guard, moving language-load/cleanup and plugin metadata updates inside the critical section, clearing stale resource tracking after cleanup, and adding a UTF‑8 BOM to Internationalization.cs. No public signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller (ChangeLanguageAsync)
participant Intl as Internationalization
participant Old as _oldResources
participant Merged as App.Resources.MergedDictionaries
participant Plugins as Plugin metadata updater
Caller->>Intl: ChangeLanguageAsync(language, updateMetadata)
note right of Intl #DFF0D8: acquire semaphore
Intl->>Intl: await _langChangeLock.WaitAsync()
Intl->>Old: RemoveOldLanguageFiles()
Intl->>Merged: remove each resource in _oldResources
Intl->>Old: _oldResources.Clear()
alt language != English
Intl->>Intl: LoadLanguage(language)
end
Intl->>Intl: ChangeCultureInfo(language.LanguageCode)
alt updateMetadata == true
Intl->>Plugins: UpdatePluginMetadataTranslations() (Task.Run)
end
note right of Intl #F8F0D8: release semaphore
Intl->>Intl: _langChangeLock.Release()
Intl-->>Caller: return
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 (3)
Flow.Launcher.Core/Resource/Internationalization.cs (3)
253-261: Critical: Do not mutate _oldResources while enumerating; clear after loop to actually fix the leak.Removing from the list you’re iterating will throw InvalidOperationException and still risks leaving references around. Remove from MergedDictionaries, then Clear() the tracking list.
Apply this diff:
private void RemoveOldLanguageFiles() { var dicts = Application.Current.Resources.MergedDictionaries; - foreach (var r in _oldResources) - { - dicts.Remove(r); - _oldResources.Remove(r); - } + foreach (var r in _oldResources) + { + dicts.Remove(r); + } + _oldResources.Clear(); }
56-70: Bug: SystemLanguageCode is always reset to 'en'.The final assignment overwrites any match found in the loop, forcing “System Language” to English.
Suggested fix:
public static void InitSystemLanguageCode() { var availableLanguages = AvailableLanguages.GetAvailableLanguages(); @@ - // Try to find a match in the available languages list - foreach (var language in availableLanguages) - { - var languageCode = language.LanguageCode; - - if (string.Equals(languageCode, twoLetterCode, StringComparison.OrdinalIgnoreCase) || - string.Equals(languageCode, threeLetterCode, StringComparison.OrdinalIgnoreCase) || - string.Equals(languageCode, fullName, StringComparison.OrdinalIgnoreCase)) - { - SystemLanguageCode = languageCode; - } - } - - SystemLanguageCode = DefaultLanguageCode; + // Default, then override on first match + SystemLanguageCode = DefaultLanguageCode; + foreach (var language in availableLanguages) + { + var languageCode = language.LanguageCode; + if (string.Equals(languageCode, twoLetterCode, StringComparison.OrdinalIgnoreCase) || + string.Equals(languageCode, threeLetterCode, StringComparison.OrdinalIgnoreCase) || + string.Equals(languageCode, fullName, StringComparison.OrdinalIgnoreCase)) + { + SystemLanguageCode = languageCode; + break; + } + } }
355-358: Don’t return from inside the foreach; use continue.Returning exits the whole method on the first non‑IPluginI18n plugin, skipping updates for the rest.
- if (p.Plugin is not IPluginI18n pluginI18N) return; + if (p.Plugin is not IPluginI18n pluginI18N) continue;
🧹 Nitpick comments (1)
Flow.Launcher.Core/Resource/Internationalization.cs (1)
1-1: UTF‑8 BOM slipped in.A BOM was introduced; keep file encoding consistent with the repo (usually UTF‑8 without BOM).
-using System; +using System;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Core/Resource/Internationalization.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). (2)
- GitHub Check: gitStream.cm
- GitHub Check: build
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Flow.Launcher.Core/Resource/Internationalization.cs (2)
56-70: Bug: SystemLanguageCode is always reset to “en”.Line 69 overwrites any detected match. Initialize to default first, then break on first match.
Proposed fix (outside changed lines):
public static void InitSystemLanguageCode() { var availableLanguages = AvailableLanguages.GetAvailableLanguages(); var currentCulture = CultureInfo.CurrentCulture; var two = currentCulture.TwoLetterISOLanguageName; var three = currentCulture.ThreeLetterISOLanguageName; var full = currentCulture.Name; SystemLanguageCode = DefaultLanguageCode; foreach (var language in availableLanguages) { var code = language.LanguageCode; if (code.Equals(two, StringComparison.OrdinalIgnoreCase) || code.Equals(three, StringComparison.OrdinalIgnoreCase) || code.Equals(full, StringComparison.OrdinalIgnoreCase)) { SystemLanguageCode = code; break; } } }
355-359: Bug: early return stops processing remaining plugins.Return inside the foreach exits the entire method if a plugin is not IPluginI18n; should be continue.
Proposed change (outside changed lines):
foreach (var p in PluginManager.GetTranslationPlugins()) { if (p.Plugin is not IPluginI18n pluginI18N) continue; try { p.Metadata.Name = pluginI18N.GetTranslatedPluginTitle(); p.Metadata.Description = pluginI18N.GetTranslatedPluginDescription(); pluginI18N.OnCultureInfoChanged(CultureInfo.CurrentCulture); } catch (Exception e) { API.LogException(ClassName, $"Failed for <{p.Metadata.Name}>", e); } }
🧹 Nitpick comments (2)
Flow.Launcher.Core/Resource/Internationalization.cs (2)
165-169: Serialize language changes and marshal resource ops to UI thread.Rapid selection changes can overlap ChangeLanguageAsync, racing on _oldResources and Application.Current.Resources.
Suggested pattern (outside changed lines):
// Field private readonly SemaphoreSlim _langChangeGate = new(1, 1); // In ChangeLanguageAsync await _langChangeGate.WaitAsync(); try { await Application.Current.Dispatcher.InvokeAsync(RemoveOldLanguageFiles); if (language != AvailableLanguages.English) await Application.Current.Dispatcher.InvokeAsync(() => LoadLanguage(language)); ChangeCultureInfo(language.LanguageCode); if (updateMetadata) await Task.Run(UpdatePluginMetadataTranslations); } finally { _langChangeGate.Release(); }Also applies to: 186-203
1-1: Unintended UTF‑8 BOM.A BOM was added. If the repo targets utf-8 without BOM (common in .editorconfig), please remove to avoid noisy diffs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Core/Resource/Internationalization.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). (3)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
Pull request was closed
|
Still need to apply some adjustments |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull Request Overview
Fixes a memory leak issue by adding synchronization to language changes and clearing old resource references properly.
- Adds thread-safe synchronization for language change operations using a semaphore
- Ensures old resource dictionaries are properly cleared from memory after removal
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Flow.Launcher.Core/Resource/Internationalization.cs (2)
57-71: System language detection always falls back to English.
SystemLanguageCodeis overwritten to"en"unconditionally at Line 70. Only set the default if no match was found.Apply this diff:
- SystemLanguageCode = DefaultLanguageCode; + if (string.IsNullOrEmpty(SystemLanguageCode)) + SystemLanguageCode = DefaultLanguageCode;
365-378: Don’t exit early when a plugin lacks IPluginI18n.
returnaborts the entire update loop; usecontinueso other plugins still update.Apply this diff:
- if (p.Plugin is not IPluginI18n pluginI18N) return; + if (p.Plugin is not IPluginI18n pluginI18N) continue;
🧹 Nitpick comments (2)
Flow.Launcher.Core/Resource/Internationalization.cs (2)
1-1: Avoid committing a UTF‑8 BOM in C# files.It creates noisy diffs and can confuse some tooling. Prefer UTF‑8 without BOM.
310-319: Fix log message variable.It logs
<{path}>when the missing file is actuallyenglish. Improves diagnostics.Apply this diff:
- API.LogError(ClassName, $"Default English Language path can't be found <{path}>"); + API.LogError(ClassName, $"Default English Language path can't be found <{english}>");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Core/Resource/Internationalization.cs(4 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
🔇 Additional comments (2)
Flow.Launcher.Core/Resource/Internationalization.cs (2)
33-33: Good call adding a semaphore to serialize language changes.This should prevent overlapping resource mutations.
263-271: LGTM: safe removal pattern.Iterating
_oldResourceswithout mutating, thenClear()eliminates the previous enumeration/mutation issue.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Flow.Launcher.Core/Resource/Internationalization.cs (2)
33-33: Lock scope: make the semaphore static (or confirm singleton) and consider disposal.If multiple Internationalization instances can exist, per‑instance locks won’t serialize language changes globally. Prefer a static semaphore or confirm there’s only one instance for app lifetime. Disposal is optional for app‑lifetime objects but silences analyzers.
Apply if needed:
- private readonly SemaphoreSlim _langChangeLock = new(1, 1); + private static readonly SemaphoreSlim _langChangeLock = new(1, 1);To verify instance count, run:
#!/bin/bash # Find construction sites of Internationalization rg -nP --type=cs -C2 '\bnew\s+Internationalization\s*\('
189-212: Catch/log exceptions to avoid unobserved task faults; avoid Task.Run here.ChangeLanguage fire‑and‑forgets this Task (Line 166). Without a catch, faults become unobserved. Also, UpdatePluginMetadataTranslations is synchronous; calling it via Task.Run adds overhead and makes thread affinity unclear.
private async Task ChangeLanguageAsync(Language language, bool updateMetadata = true) { - await _langChangeLock.WaitAsync(); + await _langChangeLock.WaitAsync(); try { // Remove old language files and load language RemoveOldLanguageFiles(); if (language != AvailableLanguages.English) { LoadLanguage(language); } // Change culture info ChangeCultureInfo(language.LanguageCode); if (updateMetadata) { - // Raise event for plugins after culture is set - await Task.Run(UpdatePluginMetadataTranslations); + // Raise event for plugins after culture is set + UpdatePluginMetadataTranslations(); // run on caller thread for predictable affinity } } + catch (Exception e) + { + API.LogException(ClassName, "ChangeLanguageAsync failed", e); + } finally { _langChangeLock.Release(); } }Also confirm UpdatePluginMetadataTranslations is safe on the calling thread (likely UI).
🧹 Nitpick comments (3)
Flow.Launcher.Core/Resource/Internationalization.cs (3)
1-1: Nit: UTF‑8 BOM churn — confirm repo policy.If the repo doesn’t mandate BOM for C# files, consider removing it to avoid diff noise.
Do we have an .editorconfig rule requiring BOM for .cs files?
203-207: Update loop exits early on non‑i18n plugins — fix premature return.UpdatePluginMetadataTranslations currently returns on the first plugin that isn’t IPluginI18n, skipping the rest. Use continue.
- if (p.Plugin is not IPluginI18n pluginI18N) return; + if (p.Plugin is not IPluginI18n pluginI18N) continue;
57-71: System language detection overwrites matches with default — set default before the loop and don’t reset after.As written, any match found is immediately overridden by the final assignment to DefaultLanguageCode.
- // Try to find a match in the available languages list + // Default to English; try to find a match in the available languages list + SystemLanguageCode = DefaultLanguageCode; foreach (var language in availableLanguages) { var languageCode = language.LanguageCode; if (string.Equals(languageCode, twoLetterCode, StringComparison.OrdinalIgnoreCase) || string.Equals(languageCode, threeLetterCode, StringComparison.OrdinalIgnoreCase) || string.Equals(languageCode, fullName, StringComparison.OrdinalIgnoreCase)) { SystemLanguageCode = languageCode; + break; } } - - SystemLanguageCode = DefaultLanguageCode;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Core/Resource/Internationalization.cs(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Flow.Launcher.Core/Resource/Internationalization.cs
⏰ 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). (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (1)
Flow.Launcher.Core/Resource/Internationalization.cs (1)
270-270: LGTM: clearing _oldResources after removals prevents lingering references.This directly addresses the memory‑leak risk when switching languages repeatedly.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@Jack251970 were you able to confirm that this fixes memory leak? Is the issue still reproducible? |
I cannot reproduce this issue from my end. But this reference issue is a noticeable error that we should take into account. |
Remove old dictionaries references to fix possible memory leak
…ion_memory_leak Remove old dictionaries references to fix possible memory leak
Fix #2785