BrowserBookmark plugin, advanced #4017
Conversation
- Service-oriented architecture - Asynchronous Operations - Cleaner file watching - MVVM / ViewModel - Browser engine auto-detect - New favicon fetch if missing, background thread, efficient fetching - Firefox now has periodic check on `places.sqlite` rather than constant watch
|
🥷 Code experts: Jack251970, Yusyuriv Jack251970, jjw24 have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
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. |
|
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. 📝 WalkthroughWalkthroughReplaced legacy synchronous bookmark loaders and helpers with an async, service-oriented implementation: new async bookmark loaders, favicon extraction/conversion/caching, file-watching, browser detection, observable settings, MVVM UI changes, and Main refactored to IAsyncReloadable with asynchronous reloads and lifecycle management. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Flow as FlowLauncher
participant Plugin as BrowserBookmark.Main
participant LoaderSvc as BookmarkLoaderService
participant Chromium as ChromiumBookmarkLoader
participant Firefox as FirefoxBookmarkLoader
participant Favicon as FaviconService
participant Watcher as BookmarkWatcherService
Flow->>Plugin: Init(context, settings)
Plugin->>Plugin: Setup temp/cache & services
Plugin->>LoaderSvc: LoadBookmarksAsync(ct)
par loaders
LoaderSvc->>Chromium: GetBookmarksAsync()
Chromium-->>LoaderSvc: bookmarks (async stream)
LoaderSvc->>Firefox: GetBookmarksAsync()
Firefox-->>LoaderSvc: bookmarks (async stream)
end
LoaderSvc-->>Plugin: (Bookmarks, DiscoveredFiles)
Plugin->>Favicon: ProcessBookmarkFavicons(bookmarks, ct)
Plugin->>Watcher: UpdateWatchers(discoveredFiles)
User->>Plugin: Query(q)
Plugin-->>User: Results
Watcher-->>Plugin: OnBookmarkFileChanged
Plugin->>Plugin: ReloadDataAsync()
sequenceDiagram
autonumber
participant Favicon as FaviconService
participant Local as LocalFaviconExtractor
participant Web as FaviconWebClient
participant Html as HtmlFaviconParser
participant Img as ImageConverter
participant Cache as FaviconCache
Favicon->>Local: GetFaviconDataAsync(bookmark)
alt local DB hit
Local-->>Favicon: bytes
Favicon->>Img: ToPngAsync(stream)
Img-->>Favicon: png + size
Favicon->>Cache: Save PNG
else local miss & web enabled
Favicon->>Web: GetHtmlHeadAsync(pageUri)
Web-->>Favicon: html + base
Favicon->>Html: Parse(html, base)
Html-->>Favicon: candidates
Favicon->>Web: DownloadFaviconAsync(candidate)
Web-->>Favicon: stream
Favicon->>Img: ToPngAsync(stream)
Img-->>Favicon: png + size
Favicon->>Cache: Save PNG
end
Favicon-->>Favicon: Set bookmark.FaviconPath
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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. 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: 12
🧹 Nitpick comments (34)
Scripts/BuilderToolbox.ps1 (6)
230-234: Dispose event jobs and process to avoid leaks.Unregister-Event does not remove the event jobs. Add Remove-Job and dispose the process.
Unregister-Event -SubscriptionId $stdOutEvent.Id Unregister-Event -SubscriptionId $stdErrEvent.Id - - return ($process.ExitCode -eq 0 -or $IgnoreErrors) + try { Remove-Job -Id $stdOutEvent.Id -Force -ErrorAction SilentlyContinue } catch {} + try { Remove-Job -Id $stdErrEvent.Id -Force -ErrorAction SilentlyContinue } catch {} + try { $process.Close() } catch {} + try { $process.Dispose() } catch {} + return ($process.ExitCode -eq 0 -or $IgnoreErrors)
327-333: Pass self-contained as a canonical boolean string.dotnet CLI expects true/false; avoid relying on PowerShell’s boolean stringification.
- $arguments = "publish `"$($global:mainProjectFile)`" -c Release -r $($global:publishRuntimeId) --self-contained $IsSelfContained -o `"$PublishDir`"" + $sc = $(if ($IsSelfContained) { 'true' } else { 'false' }) + $arguments = "publish `"$($global:mainProjectFile)`" -c Release -r $($global:publishRuntimeId) --self-contained $sc -o `"$PublishDir`""
380-382: Fix quoting for git pretty format on Windows.Use double quotes for the format string; single quotes can be unreliable on Windows.
- $gitLogCommand = "log $commitRange --pretty=format:'* %h - %s (%an)'" + $gitLogCommand = "log $commitRange --pretty=format:`"* %h - %s (%an)`"" $changelogContent = (git -C $global:solutionRoot $gitLogCommand)
655-663: Prefer JSON parsing to remove createdump from deps.json instead of regex.Text regex is brittle and risks corrupting JSON. Parse and edit the JSON, then serialize back.
If desired, I can provide a JSON-editing implementation tailored to the deps.json structure used here.
136-161: Use dotnet tool update for VPK instead of uninstall/install.Simpler and preserves settings; still pins to the required version.
- Invoke-DotnetCommand -Command "tool" -Arguments "uninstall -g vpk" -IgnoreErrors $true - if (-not (Invoke-DotnetCommand -Command "tool" -Arguments "install -g vpk --version $requiredVpkVersion")) { + if (-not (Invoke-DotnetCommand -Command "tool" -Arguments "update -g vpk --version $requiredVpkVersion")) { Write-Host "Failed to install required VPK version." -ForegroundColor Red return $false }
113-133: Read SDK version from global.json and verify SolutionAssemblyInfo.cs path
- In Scripts/BuilderToolbox.ps1, replace
with logic to parse$global:requiredDotNetVersion = "9".sdk.versionfrom the repo’s global.json (falling back as needed).- Immediately after
add a$global:assemblyInfoPath = Join-Path $solutionRoot "SolutionAssemblyInfo.cs"Test-Pathcheck and error out if the file doesn’t exist.Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs (1)
46-59: Avoid rebuilding the entire HTML string each loop.Calling
contentBuilder.ToString()on every iteration re-materializes the full head (up to ~500 KB) and makes the loop O(n²). Scan only the recent tail instead.- var contentBuilder = new StringBuilder(); + const string HeadCloseTag = "</head>"; + var contentBuilder = new StringBuilder(); var buffer = new char[4096]; int charsRead; var totalCharsRead = 0; const int maxCharsToRead = 500 * 1024; while (!token.IsCancellationRequested && (charsRead = await reader.ReadAsync(buffer, 0, buffer.Length)) > 0 && totalCharsRead < maxCharsToRead) { contentBuilder.Append(buffer, 0, charsRead); totalCharsRead += charsRead; - if (contentBuilder.ToString().Contains("</head>", StringComparison.OrdinalIgnoreCase)) - { - break; - } + var scanLength = Math.Min(contentBuilder.Length, buffer.Length + HeadCloseTag.Length); + if (scanLength >= HeadCloseTag.Length) + { + var segment = contentBuilder.ToString(contentBuilder.Length - scanLength, scanLength); + if (segment.Contains(HeadCloseTag, StringComparison.OrdinalIgnoreCase)) + { + break; + } + } }Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (2)
57-60: Ensure temp directory exists and avoid duplicate entries after fallback
- Create the temp folder before copying.
- Clear partially-read results before re-reading from the copy to avoid duplicates.
Apply this diff:
- tempDbPath = Path.Combine(_tempPath, $"ff_places_{Guid.NewGuid()}.sqlite"); - File.Copy(_placesPath, tempDbPath, true); - await ReadBookmarksFromDb(tempDbPath, bookmarks, cancellationToken); + tempDbPath = Path.Combine(_tempPath, $"ff_places_{Guid.NewGuid()}.sqlite"); + Directory.CreateDirectory(_tempPath); + File.Copy(_placesPath, tempDbPath, true); + bookmarks.Clear(); // avoid duplicates if direct-read partially succeeded + await ReadBookmarksFromDb(tempDbPath, bookmarks, cancellationToken);
95-104: Use typed getters for SQLite columnsTyped access reduces overhead and avoids string dictionary lookups.
Apply this diff:
- var title = reader["title"]?.ToString() ?? string.Empty; - var url = reader["url"]?.ToString(); + var url = !reader.IsDBNull(0) ? reader.GetString(0) : null; + var title = !reader.IsDBNull(1) ? reader.GetString(1) : string.Empty;Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkWatcherService.cs (3)
24-31: Unsubscribe events before disposing watchers to prevent handler leaksDispose alone doesn’t detach delegates; unsubscribe explicitly.
Apply this diff:
- foreach (var watcher in _watchers) - { - watcher.EnableRaisingEvents = false; - watcher.Dispose(); - } + foreach (var watcher in _watchers) + { + watcher.EnableRaisingEvents = false; + watcher.Changed -= OnFileChanged; + watcher.Created -= OnFileChanged; + watcher.Deleted -= OnFileChanged; + watcher.Renamed -= OnFileRenamed; + watcher.Dispose(); + }
19-20: Guard timer callback after disposalRace: a scheduled callback could fire after Dispose. Check
_disposedinside the callback.Apply this diff:
- _debounceTimer = new Timer(_ => OnBookmarkFileChanged?.Invoke(), null, Timeout.Infinite, Timeout.Infinite); + _debounceTimer = new Timer(_ => + { + lock (_lock) + { + if (_disposed) return; + } + OnBookmarkFileChanged?.Invoke(); + }, null, Timeout.Infinite, Timeout.Infinite);
41-46: Increase FileSystemWatcher buffer to reduce overflow under bursty writesDefault buffer (8 KB) can overflow easily for frequent bookmark updates.
Apply this diff:
var watcher = new FileSystemWatcher(directory) { Filter = fileName, - NotifyFilter = NotifyFilters.LastWrite | NotifyFilters.FileName | NotifyFilters.Size + NotifyFilter = NotifyFilters.LastWrite | NotifyFilters.FileName | NotifyFilters.Size, + InternalBufferSize = 64 * 1024 };Plugins/Flow.Launcher.Plugin.BrowserBookmark/ViewModels/CustomBrowserSettingViewModel.cs (1)
72-79: Optional: also require a non-empty name to enable SavePrevents saving nameless entries.
If desired, introduce a computed property and update the command:
- [RelayCommand(CanExecute = nameof(IsValidPath))] + [RelayCommand(CanExecute = nameof(CanSave))] private void Save() { _originalBrowser.Name = EditableBrowser.Name; _originalBrowser.DataDirectoryPath = EditableBrowser.DataDirectoryPath; _originalBrowser.BrowserType = EditableBrowser.BrowserType; _closeAction(true); } + + public bool CanSave => IsValidPath && !string.IsNullOrWhiteSpace(EditableBrowser.Name);Also call
SaveCommand.NotifyCanExecuteChanged()whenNamechanges (subscribe similarly toDataDirectoryPath).Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/HtmlFaviconParser.cs (1)
57-57: Optional: use class name as log tagImproves log filtering consistency.
Apply this diff:
- _context.API.LogDebug(nameof(Parse), $"Found potential favicon link. Raw tag: '{linkTag}', Extracted href: '{href}', Base URI: '{effectiveBaseUri}'"); + _context.API.LogDebug(nameof(HtmlFaviconParser), $"Found potential favicon link. Raw tag: '{linkTag}', Extracted href: '{href}', Base URI: '{effectiveBaseUri}'");Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs (2)
33-39: Use a single source of truth for the selected itemMixing
SelectedCustomBrowserandCustomBrowsers.SelectedItemis inconsistent. Prefer the bound property and fall back to control only if necessary.Apply this diff:
- if (CustomBrowsers.SelectedItem is CustomBrowser selectedCustomBrowser) - { - Settings.CustomBrowsers.Remove(selectedCustomBrowser); - } + var selected = SelectedCustomBrowser ?? CustomBrowsers.SelectedItem as CustomBrowser; + if (selected is not null) + { + Settings.CustomBrowsers.Remove(selected); + }Ensure
SelectedCustomBrowseris bound to the list’sSelectedItemin XAML.
65-67: Optional: set window owner for modality/focusHelps keep the dialog centered and modal to Settings.
Apply this diff:
- var window = new CustomBrowserSetting(SelectedCustomBrowser); + var window = new CustomBrowserSetting(SelectedCustomBrowser) { Owner = Window.GetWindow(this) };Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxProfileFinder.cs (2)
17-18: Remove redundant ternary.
GetPlacesPathFromProfileDiralready returns null when not found; return it directly.- return !string.IsNullOrEmpty(placesPath) ? placesPath : null; + return placesPath;
53-64: Limit the [Install...] parsing to its section to avoid cross‑section matches.Current regex can capture Default from later sections under rare INI layouts. Parse the [Install...] block first, then read Default within it.
- var installMatch = Regex.Match(iniContent, @"^\[Install[^\]]+\](?:.|\n|\r)*?^Default\s*=\s*(.+)", RegexOptions.Multiline | RegexOptions.IgnoreCase); - if (installMatch.Success) + var installSection = Regex.Match(iniContent, @"^\[Install[^\]]+\](?:.|\n|\r)*?(?=\r?\n\[|$)", RegexOptions.Multiline | RegexOptions.IgnoreCase); + if (installSection.Success) { - var path = installMatch.Groups[1].Value.Trim(); + var defaultMatch = Regex.Match(installSection.Value, @"^Default\s*=\s*(.+)", RegexOptions.Multiline | RegexOptions.IgnoreCase); + if (defaultMatch.Success) + { + var path = defaultMatch.Groups[1].Value.Trim(); // This path is typically relative, e.g., "Profiles/xyz.default-release" var profilePath = Path.Combine(profileFolderPath, path.Replace('/', Path.DirectorySeparatorChar)); var placesDb = Path.Combine(profilePath, "places.sqlite"); if (File.Exists(placesDb)) { return placesDb; } + } }Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (2)
53-58: Preserve aspect ratio for SVG scaling.Non‑uniform x/y scaling can distort icons. Use uniform scale and center.
- using var bitmap = new SKBitmap(TargetIconSize, TargetIconSize); - using var canvas = new SKCanvas(bitmap); - canvas.Clear(SKColors.Transparent); - var scaleMatrix = SKMatrix.CreateScale((float)TargetIconSize / svg.Picture.CullRect.Width, (float)TargetIconSize / svg.Picture.CullRect.Height); - canvas.DrawPicture(svg.Picture, in scaleMatrix); + var w = svg.Picture.CullRect.Width; + var h = svg.Picture.CullRect.Height; + var scale = MathF.Min(TargetIconSize / w, TargetIconSize / h); + var tx = (TargetIconSize - w * scale) / 2f; + var ty = (TargetIconSize - h * scale) / 2f; + + using var bitmap = new SKBitmap(TargetIconSize, TargetIconSize); + using var canvas = new SKCanvas(bitmap); + canvas.Clear(SKColors.Transparent); + canvas.Translate(tx, ty); + canvas.Scale(scale); + canvas.DrawPicture(svg.Picture);
23-44: Honor cancellation between conversion attempts.Check the token before each attempt to bail out quickly.
var (pngData, size) = TryConvertSvgToPng(ms); if (pngData is not null) return (pngData, size); + token.ThrowIfCancellationRequested(); ms.Position = 0; (pngData, size) = TryConvertIcoToPng(ms); if (pngData is not null) return (pngData, size); + token.ThrowIfCancellationRequested(); ms.Position = 0; (pngData, size) = TryConvertBitmapToPng(ms); return (pngData, size);Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (2)
137-140: Avoid duplicate Firefox loaders when MSI and MSIX point to the same places.sqlite.Guard against yielding both if paths are equal.
- if (!string.IsNullOrEmpty(msixPlacesPath)) + if (!string.IsNullOrEmpty(msixPlacesPath) && !string.Equals(msixPlacesPath, placesPath, StringComparison.OrdinalIgnoreCase)) { yield return new FirefoxBookmarkLoader("Firefox (Store)", msixPlacesPath, _tempPath, logAction); }
53-54: Align dedup logic with desired equality
Distinct() uses Bookmark’s record-generatedEquals(all four properties) and its customGetHashCode(Name + Url). If you intend to dedupe by Url only, switch tobookmarks.DistinctBy(b => b.Url)or supply anIEqualityComparer<Bookmark>that compares Url.Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs (2)
49-62: Broaden error handling to avoid aborting on IO issues.File.OpenRead and ParseAsync can throw IOException/UnauthorizedAccessException; catch them per profile to continue.
- catch (JsonException ex) + catch (JsonException ex) { _logException(nameof(ChromiumBookmarkLoader), $"Failed to parse bookmarks file for {_browserName}: {bookmarkPath}", ex); } + catch (IOException ex) + { + _logException(nameof(ChromiumBookmarkLoader), $"IO error reading bookmarks file for {_browserName}: {bookmarkPath}", ex); + } + catch (UnauthorizedAccessException ex) + { + _logException(nameof(ChromiumBookmarkLoader), $"Access denied reading bookmarks file for {_browserName}: {bookmarkPath}", ex); + }
93-118: Optional: be cancellation-friendly in deep trees.Check cancellation periodically within the child traversal to improve responsiveness.
Would you like a patch adding token checks inside the recursion?
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/CustomBrowserSetting.xaml (2)
129-129: Update binding to push changes as the user types.Enable immediate VM updates for name input.
- Text="{Binding EditableBrowser.Name}" /> + Text="{Binding EditableBrowser.Name, UpdateSourceTrigger=PropertyChanged}" />
156-156: Update binding to push path changes as the user types.Helps live engine detection feedback.
- Text="{Binding EditableBrowser.DataDirectoryPath}" /> + Text="{Binding EditableBrowser.DataDirectoryPath, UpdateSourceTrigger=PropertyChanged}" />Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (1)
23-30: Do not infer Firefox by substring; detect by DB presence (or explicit type).Relying on Source.Contains("Firefox") misclassifies custom Firefox-based browsers (e.g., Librewolf, Waterfox) and silently falls back to the Chromium query path, lowering local favicon hit rate. Prefer checking for the presence of favicons.sqlite/Favicons in the profile path (and/or an explicit BrowserType on Bookmark).
Apply this diff to make detection robust and case-insensitive with DB presence fallback:
- public async Task<byte[]?> GetFaviconDataAsync(Bookmark bookmark, CancellationToken token) - { - return bookmark.Source switch - { - var s when s.Contains("Firefox") => await GetFirefoxFaviconAsync(bookmark, token), - _ => await GetChromiumFaviconAsync(bookmark, token) // Default to Chromium - }; - } + public Task<byte[]?> GetFaviconDataAsync(Bookmark bookmark, CancellationToken token) + { + var firefoxDb = Path.Combine(bookmark.ProfilePath, "favicons.sqlite"); + var chromiumDb = Path.Combine(bookmark.ProfilePath, "Favicons"); + + if (File.Exists(firefoxDb)) + return GetFirefoxFaviconAsync(bookmark, token); + if (File.Exists(chromiumDb)) + return GetChromiumFaviconAsync(bookmark, token); + + // Heuristic fallback if neither DB is present + if (!string.IsNullOrEmpty(bookmark.Source) && + bookmark.Source.Contains("Firefox", StringComparison.OrdinalIgnoreCase)) + return GetFirefoxFaviconAsync(bookmark, token); + + return GetChromiumFaviconAsync(bookmark, token); + }Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (2)
100-107: Cache key mismatch: per-URL check vs per-host write. Unify to host-level.ProcessBookmarkFavicons checks cache with a URL-hash, but FetchAndCacheFaviconAsync writes per-host. This causes repeated work on every run for bookmarks without local favicons.
Apply this diff to check/write with a host-based key from the start:
- var cachePath = GetCachePath(bookmark.Url, _faviconCacheDir); + var cacheKey = Uri.TryCreate(bookmark.Url, UriKind.Absolute, out var uriForKey) + ? uriForKey.GetLeftPart(UriPartial.Authority) + : bookmark.Url; + var cachePath = GetCachePath(cacheKey, _faviconCacheDir); if (File.Exists(cachePath)) { bookmark.FaviconPath = cachePath; return; }This also ensures local DB extractions are stored under the same per-host cache as web fetches.
232-240: Minor: clarify SelectBestFavicon default path.The Size default is 0, so icoResult is chosen even when both are “no data.” It’s harmless because you check PngData later, but consider using -1 as the default Size to make intent explicit.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/en.xaml (2)
8-8: Minor: description clarity.Consider clarifying that web retrieval is optional and local DB extraction is attempted first, e.g., “Search your browser bookmarks; load favicons from your browser (and optionally the web).”
16-17: Nit: “Others...” vs “Other browsers…”If this toggles custom browsers, “Other browsers…” may be clearer. Optional.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (3)
73-74: Use Volatile.Read for the bookmarks snapshot.To pair with Volatile.Write in ReloadDataAsync and avoid visibility issues under concurrency, read with Volatile.Read.
Apply this diff:
- var bookmarks = _bookmarks; // use a local copy + var bookmarks = Volatile.Read(ref _bookmarks); // atomic snapshot
52-68: Temp directory setup fallback.If Temp deletion/creation fails, subsequent operations may break. Consider falling back to Path.GetTempPath() on failure.
236-261: Context menu: dual icon sources.You set both Glyph and IcoPath. If both are supported, fine; otherwise prefer one to avoid UI inconsistencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromeBookmarkLoader.cs(0 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs(0 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Commands/BookmarkLoader.cs(0 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/CustomChromiumBookmarkLoader.cs(0 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/CustomFirefoxBookmarkLoader.cs(0 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/EdgeBookmarkLoader.cs(0 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs(0 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs(0 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/IBookmarkLoader.cs(0 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/en.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs(2 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/BaseModel.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Bookmark.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs(2 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkWatcherService.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxProfileFinder.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/HtmlFaviconParser.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/IBookmarkLoader.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/ViewModels/CustomBrowserSettingViewModel.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/CustomBrowserSetting.xaml(8 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/CustomBrowserSetting.xaml.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml(5 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs(3 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/plugin.json(1 hunks)Scripts/BuilderToolbox.ps1(1 hunks)
💤 Files with no reviewable changes (9)
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/CustomChromiumBookmarkLoader.cs
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/CustomFirefoxBookmarkLoader.cs
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromeBookmarkLoader.cs
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/EdgeBookmarkLoader.cs
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Commands/BookmarkLoader.cs
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/IBookmarkLoader.cs
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-06T05:26:32.163Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3664
File: Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs:326-326
Timestamp: 2025-06-06T05:26:32.163Z
Learning: In the Firefox bookmark loader (FirefoxBookmarkLoader.cs), using hardcoded Windows line endings ("\r\n") for splitting profiles.ini content is acceptable and platform-agnostic line splitting is not required.
Applied to files:
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.csPlugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxProfileFinder.cs
📚 Learning: 2024-10-08T15:52:58.573Z
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
Applied to files:
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj
📚 Learning: 2025-09-13T06:04:26.977Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3593
File: Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs:143-165
Timestamp: 2025-09-13T06:04:26.977Z
Learning: In Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml, the ListView_SizeChanged handler in the C# file assumes 5 GridViewColumns but the XAML may have more columns. The user Jack251970 indicated this mismatch doesn't need to be fixed when pointed out during PR #3593 review.
Applied to files:
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml
📚 Learning: 2025-09-11T08:30:29.487Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3593
File: Flow.Launcher/SettingPages/Views/SettingsPaneTheme.xaml:0-0
Timestamp: 2025-09-11T08:30:29.487Z
Learning: In Flow.Launcher's SettingsPaneTheme.xaml theme editor panel, the inner ui:ScrollViewerEx within the Border (Grid.Column="1") is intentionally kept because its height cannot be expanded to the whole page due to layout constraints. This serves a different scrolling purpose than the outer ScrollViewerEx and is necessary for the theme editor panel functionality.
Applied to files:
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml
🧬 Code graph analysis (17)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/CustomBrowserSetting.xaml.cs (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ViewModels/CustomBrowserSettingViewModel.cs (2)
CustomBrowserSettingViewModel(14-101)CustomBrowserSettingViewModel(43-54)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs (1)
CustomBrowser(7-51)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs (1)
BrowserType(29-49)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkWatcherService.cs (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (2)
OnBookmarkFileChanged(138-141)Dispose(265-274)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (4)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Settings(5-51)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (3)
Task(85-105)FirefoxBookmarkLoader(13-106)FirefoxBookmarkLoader(31-37)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs (2)
ChromiumBookmarkLoader(14-120)ChromiumBookmarkLoader(23-29)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxProfileFinder.cs (4)
FirefoxProfileFinder(9-114)GetFirefoxPlacesPath(11-18)GetFirefoxMsixPlacesPath(20-40)GetPlacesPathFromProfileDir(42-113)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (4)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (2)
IBookmarkLoader(152-165)Task(26-54)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/IBookmarkLoader.cs (1)
IAsyncEnumerable(10-10)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (5)
Task(23-30)Task(32-41)Task(43-53)Task(55-91)Task(93-106)Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs (1)
FirefoxBookmarkLoaderBase(14-204)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (6)
Task(75-92)Task(94-132)Task(134-150)Task(163-230)Task(242-263)Task(265-278)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (1)
Task(85-105)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (7)
Task(75-92)Task(94-132)Task(134-150)Task(163-230)Task(242-263)Task(265-278)Dispose(280-288)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (1)
Task(23-44)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/HtmlFaviconParser.cs (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (2)
FaviconService(17-289)FaviconService(36-54)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (2)
ImageConverter(13-129)ImageConverter(18-21)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (6)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Settings(5-51)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (2)
BookmarkLoaderService(13-166)BookmarkLoaderService(19-24)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (3)
FaviconService(17-289)FaviconService(36-54)Dispose(280-288)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkWatcherService.cs (4)
BookmarkWatcherService(7-94)BookmarkWatcherService(17-20)UpdateWatchers(22-53)Dispose(76-93)Flow.Launcher.Core/Plugin/PluginManager.cs (5)
List(488-515)Task(94-112)Task(114-122)Task(124-131)Task(133-140)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs (2)
SettingsControl(9-68)SettingsControl(14-18)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxProfileFinder.cs (2)
FirefoxProfileFinder(9-114)GetPlacesPathFromProfileDir(42-113)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/IBookmarkLoader.cs (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (1)
IBookmarkLoader(152-165)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs (3)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs (1)
CustomBrowser(7-51)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/CustomBrowserSetting.xaml.cs (2)
CustomBrowserSetting(11-37)CustomBrowserSetting(15-24)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Settings(5-51)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (6)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Settings(5-51)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (7)
LocalFaviconExtractor(12-136)LocalFaviconExtractor(17-21)Task(23-30)Task(32-41)Task(43-53)Task(55-91)Task(93-106)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs (5)
FaviconWebClient(13-131)FaviconWebClient(19-32)Task(34-74)Task(76-124)Dispose(126-130)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/HtmlFaviconParser.cs (3)
HtmlFaviconParser(10-105)HtmlFaviconParser(25-28)List(30-81)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (6)
ImageConverter(13-129)ImageConverter(18-21)Task(23-44)PngData(46-67)PngData(69-102)PngData(104-128)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (6)
Task(110-121)Task(161-217)List(70-92)List(234-263)Result(94-108)Dispose(265-274)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (6)
Task(75-92)Task(94-132)Task(134-150)Task(163-230)Task(242-263)Task(265-278)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs (2)
Task(34-74)Task(76-124)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs (3)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (4)
IBookmarkLoader(152-165)IEnumerable(56-59)IEnumerable(61-106)IEnumerable(108-150)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/IBookmarkLoader.cs (1)
IAsyncEnumerable(10-10)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs (2)
BrowserDetector(9-50)IEnumerable(11-27)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/BaseModel.cs (2)
BaseModel(8-16)OnPropertyChanged(12-15)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs (1)
CustomBrowser(7-51)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ViewModels/CustomBrowserSettingViewModel.cs (3)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs (1)
CustomBrowser(7-51)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs (2)
BrowserType(29-49)BrowserDetector(9-50)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/BaseModel.cs (1)
OnPropertyChanged(12-15)
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs
[warning] 128-128:
msix is not a recognized word. (unrecognized-spelling)
[warning] 139-139:
msix is not a recognized word. (unrecognized-spelling)
[warning] 137-137:
msix is not a recognized word. (unrecognized-spelling)
[warning] 131-131:
msix is not a recognized word. (unrecognized-spelling)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs
[warning] 246-246:
copylink is not a recognized word. (unrecognized-spelling)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/plugin.json
[warning] 6-6:
dcog is not a recognized word. (unrecognized-spelling)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs
[warning] 143-143:
Cooldown is not a recognized word. (unrecognized-spelling)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs
[warning] 3-3:
Skia is not a recognized word. (unrecognized-spelling)
⏰ 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
🔇 Additional comments (17)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj (2)
107-109: LGTM on the ProjectReference regrouping.Thanks for keeping the project reference intact while reorganizing the item groups; this remains correct.
86-99: Icon loading unaffected by build-action change
Plugin code and plugin.json referenceImages\bookmark.pngandImages\copylink.pngvia file paths (IcoPath) and rely onCopyToOutputDirectory, not XAML pack URIs. Switching to<None Update="…">has no impact on runtime loading.Plugins/Flow.Launcher.Plugin.BrowserBookmark/plugin.json (1)
5-12: Manifest updates look good.New description, author, and language metadata all align with the revamped plugin surface.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs (1)
11-47: Defaulting to Unknown browser type is sensible.Letting detection/UI drive the browser choice avoids accidentally treating every new custom entry as Chromium.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Bookmark.cs (1)
6-19: New bookmark shape looks consistent.Adding
ProfilePathwhile basing equality onName+Urlkeeps deduping behaviour stable and gives the loaders the extra context they need.Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/IBookmarkLoader.cs (1)
7-11: Interface contract reads clean.
Nameplus async enumeration is exactly the surface the loader service needs—nice.Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
14-48: Property change notifications wired correctlyThe backing fields plus OnPropertyChanged pattern keep the UI bindings reactive as we toggle the various loader/favicon options. Nice cleanup.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/BaseModel.cs (1)
7-16: Shared INotifyPropertyChanged base is welcomeThis lightweight base keeps the models aligned with the new MVVM flow without dragging in extra dependencies. Looks good.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml (1)
103-117: Favicon options binding matches the new settingsBinding
FetchMissingFavicons’ IsEnabled toSettings.EnableFaviconsgives the right UX and keeps everything in sync with the settings model.Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs (1)
11-49: Centralized detection logic looks solidThe staged checks for Chromium Bookmarks and Firefox places.sqlite cover the common cases cleanly and reuse the existing Firefox finder—nice addition.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/HtmlFaviconParser.cs (2)
14-23: No action required: GeneratedRegex is supported under net9.0-windows. The BrowserBookmark plugin targets net9.0-windows, which fully supports the GeneratedRegex attribute.
64-67: No change required: API uses LogWarn
The IPublicAPI interface and PublicAPIInstance implement LogWarn(), so LogWarning is not defined.Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (2)
69-101: Verify WPF IconBitmapDecoder usage off the UI thread.WPF imaging types can require STA; this code may run on background thread. Confirm this path is safe in your hosting context or guard with a fallback.
Would you like me to provide a STA-dispatch wrapper or an alternative ICO decode path (Skia-only)?
11-21: No field name mismatch
rg search shows only_imageConverterin the codebase; there are no_image_converterreferences.Likely an incorrect or invalid review comment.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/CustomBrowserSetting.xaml (1)
1-18: Confirm runtime DataContext initialization.Design-time context is set; ensure code-behind or caller sets the VM at runtime.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (1)
93-106: LGTM: gzip favicon handling.GZip header detection and decompression path look correct; exceptions bubble to caller and are logged there.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (1)
216-217: Good: favicon processing limited to the delta.Processing only the newly loaded Firefox bookmarks reduces work and IO.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs
Outdated
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs
Outdated
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs
Outdated
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ViewModels/CustomBrowserSettingViewModel.cs
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/CustomBrowserSetting.xaml.cs
Show resolved
Hide resolved
Jack251970
added
the
review in progress
There was a problem hiding this comment.
Pull Request Overview
This PR completely rewrites the Browser Bookmark plugin with an advanced, service-oriented architecture that significantly improves performance, reliability, and user experience. The changes replace the legacy monolithic design with a modern, decoupled system featuring comprehensive favicon support and robust error handling.
- Replaced synchronous, tightly-coupled loaders with asynchronous, service-based architecture
- Added multi-tiered favicon retrieval system with local database extraction, web fetching, and intelligent caching
- Implemented thread-safe file watching with debounced refresh logic and periodic Firefox bookmark updates
Reviewed Changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Main.cs | Complete rewrite with async/await patterns, service injection, and improved lifecycle management |
| Services/*.cs | New service layer with specialized classes for bookmark loading, favicon processing, and file watching |
| ViewModels/CustomBrowserSettingViewModel.cs | New MVVM pattern implementation with automatic browser type detection |
| Models/*.cs | Enhanced models with nullable annotations and improved property change notifications |
| Views/*.cs | Updated UI components with cleaner data binding and simplified code-behind |
Comments suppressed due to low confidence (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkWatcherService.cs:1
- The exception handling in the Dispose method swallows the exception from line 74, and the timer operations on lines 75-76 are outside the catch block. Move the timer cleanup inside a finally block or handle the exception appropriately.
using System;
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs
Outdated
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs
Outdated
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ViewModels/CustomBrowserSettingViewModel.cs
Outdated
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs (1)
51-62: Avoid breaking persisted settings: assign explicit enum values.Reordering the enum to insert Unknown first changes underlying numeric values. If settings serialize enums as numbers (default for System.Text.Json without JsonStringEnumConverter), existing persisted Chromium(0)/Firefox(1) values would deserialize to Unknown/Chromium respectively. Assign explicit values to preserve compatibility.
Apply:
[EnumLocalize] public enum BrowserType { - [EnumLocalizeValue("Unknown")] - Unknown, - - [EnumLocalizeValue("Chromium")] - Chromium, - - [EnumLocalizeValue("Firefox")] - Firefox, + [EnumLocalizeValue("Unknown")] + Unknown = 2, + + [EnumLocalizeValue("Chromium")] + Chromium = 0, + + [EnumLocalizeValue("Firefox")] + Firefox = 1, }Additionally, consider migrating settings to string‑based enum serialization to avoid future reorder hazards.
🧹 Nitpick comments (8)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ViewModels/CustomBrowserSettingViewModel.cs (1)
86-110: Optional: Prefer a true folder picker for UX.If acceptable to add a dependency, consider CommonOpenFileDialog (Windows API Code Pack) for a proper folder picker instead of the “fake file name” trick.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs (2)
51-60: Avoid O(n^2) string growth when searching for .contentBuilder.ToString() each loop re-materializes the full string. Scan only the newly appended segment.
Apply this diff:
- if (contentBuilder.ToString().Contains("</head>", StringComparison.OrdinalIgnoreCase)) - { - break; - } + // Search only the recent segment (with overlap) to avoid O(n^2) behavior. + var start = Math.Max(0, contentBuilder.Length - buffer.Length - 16); + var len = contentBuilder.Length - start; + if (len > 0 && contentBuilder.ToString(start, len) + .IndexOf("</head>", StringComparison.OrdinalIgnoreCase) >= 0) + { + break; + }
82-87: Gate downloads by Content-Type to skip HTML and other non-images.Preemptively reject non-image payloads.
Apply this diff:
if (!response.IsSuccessStatusCode) return null; + var mediaType = response.Content.Headers.ContentType?.MediaType; + if (!string.IsNullOrEmpty(mediaType) && + !mediaType.StartsWith("image/", StringComparison.OrdinalIgnoreCase) && + !string.Equals(mediaType, "application/octet-stream", StringComparison.OrdinalIgnoreCase)) + { + return null; + }Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (1)
113-119: Dispose the MemoryStream created from localData.Prevent unnecessary allocations lingering in parallel loops.
Apply this diff:
- var (pngData, _) = await _imageConverter.ToPngAsync(new MemoryStream(localData), token); + using var ms = new MemoryStream(localData); + var (pngData, _) = await _imageConverter.ToPngAsync(ms, token);Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs (3)
44-46: Make “Default” comparison case-insensitive.Be robust against casing differences when labeling the source.
- var source = _browserName + (Path.GetFileName(profilePath) == "Default" ? "" : $" ({Path.GetFileName(profilePath)})"); + var profileName = Path.GetFileName(profilePath); + var source = _browserName + (string.Equals(profileName, "Default", StringComparison.OrdinalIgnoreCase) ? "" : $" ({profileName})");
92-111: Guard JsonElement.GetString() with ValueKind checks.Non-conforming data can make ValueKind != String and GetString() will throw. Defensive checks keep the async stream resilient.
- if (subElement.TryGetProperty("type", out var type)) + if (subElement.TryGetProperty("type", out var type) && type.ValueKind == JsonValueKind.String) { - switch (type.GetString()) + var typeStr = type.GetString(); + switch (typeStr) { case "folder": case "workspace": // Edge Workspace EnumerateFolderBookmark(subElement, bookmarks, source, profilePath); break; case "url": - if (subElement.TryGetProperty("name", out var name) && - subElement.TryGetProperty("url", out var url) && - !string.IsNullOrEmpty(name.GetString()) && - !string.IsNullOrEmpty(url.GetString())) + var hasName = subElement.TryGetProperty("name", out var name) && name.ValueKind == JsonValueKind.String; + var hasUrl = subElement.TryGetProperty("url", out var url) && url.ValueKind == JsonValueKind.String; + if (hasName && hasUrl) { - bookmarks.Add(new Bookmark(name.GetString()!, url.GetString()!, source, profilePath)); + var nameStr = name.GetString(); + var urlStr = url.GetString(); + if (!string.IsNullOrEmpty(nameStr) && !string.IsNullOrEmpty(urlStr)) + { + bookmarks.Add(new Bookmark(nameStr!, urlStr!, source, profilePath)); + } } break; } } else { _logException(nameof(ChromiumBookmarkLoader), "type property not found in bookmark node.", null); }
63-67: Optional: stream bookmarks as you parse to reduce memory.Yield bookmarks directly during traversal instead of buffering the whole list per profile. This lowers peak memory and time-to-first-result.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/CustomBrowserSetting.xaml.cs (1)
24-36: Mark handled to avoid double execution on Enter/Escape.Without e.Handled = true, default button bindings may also fire, causing duplicate Save/Cancel.
private void WindowKeyDown(object sender, KeyEventArgs e) { if (e.Key == Key.Enter) { if (_viewModel.SaveCommand.CanExecute(null)) { _viewModel.SaveCommand.Execute(null); + e.Handled = true; } } else if (e.Key == Key.Escape) { _viewModel.CancelCommand.Execute(null); + e.Handled = true; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs(2 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs(2 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/ViewModels/CustomBrowserSettingViewModel.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/CustomBrowserSetting.xaml.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-06T05:26:32.163Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3664
File: Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs:326-326
Timestamp: 2025-06-06T05:26:32.163Z
Learning: In the Firefox bookmark loader (FirefoxBookmarkLoader.cs), using hardcoded Windows line endings ("\r\n") for splitting profiles.ini content is acceptable and platform-agnostic line splitting is not required.
Applied to files:
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs
📚 Learning: 2024-11-03T07:34:24.926Z
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:34:24.926Z
Learning: In Windows Forms dialogs, Windows handles invalid paths and prevents the user from clicking "Ok" if the path is incorrect, so additional path validation is unnecessary.
Applied to files:
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ViewModels/CustomBrowserSettingViewModel.cs
📚 Learning: 2024-11-03T07:40:11.014Z
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
Applied to files:
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ViewModels/CustomBrowserSettingViewModel.cs
🧬 Code graph analysis (10)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (3)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (2)
Task(107-118)Task(158-214)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (6)
Task(75-92)Task(94-132)Task(134-159)Task(172-239)Task(251-272)Task(274-287)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (2)
Task(86-107)CleanupTempFiles(109-136)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (4)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (2)
IBookmarkLoader(152-165)Task(26-54)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs (1)
IAsyncEnumerable(30-68)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/IBookmarkLoader.cs (1)
IAsyncEnumerable(10-10)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (6)
CleanupTempFiles(114-141)Task(23-30)Task(32-41)Task(43-53)Task(55-97)Task(99-112)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs (4)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (3)
Task(107-118)Task(158-214)Dispose(262-271)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (7)
Task(75-92)Task(94-132)Task(134-159)Task(172-239)Task(251-272)Task(274-287)Dispose(289-297)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (5)
Task(23-30)Task(32-41)Task(43-53)Task(55-97)Task(99-112)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (1)
Task(23-44)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/CustomBrowserSetting.xaml.cs (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ViewModels/CustomBrowserSettingViewModel.cs (2)
CustomBrowserSettingViewModel(13-111)CustomBrowserSettingViewModel(42-53)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs (1)
CustomBrowser(5-49)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs (1)
BrowserType(29-49)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs (3)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs (1)
CustomBrowser(5-49)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/CustomBrowserSetting.xaml.cs (2)
CustomBrowserSetting(9-38)CustomBrowserSetting(13-22)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Settings(5-51)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (7)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Settings(5-51)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (2)
BookmarkLoaderService(13-166)BookmarkLoaderService(19-24)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (9)
FaviconService(17-298)FaviconService(36-54)Task(75-92)Task(94-132)Task(134-159)Task(172-239)Task(251-272)Task(274-287)Dispose(289-297)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkWatcherService.cs (4)
BookmarkWatcherService(7-94)BookmarkWatcherService(17-20)UpdateWatchers(22-53)Dispose(76-93)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (1)
Task(86-107)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (5)
Task(23-30)Task(32-41)Task(43-53)Task(55-97)Task(99-112)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs (2)
SettingsControl(8-67)SettingsControl(13-17)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (6)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Settings(5-51)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (7)
LocalFaviconExtractor(12-142)LocalFaviconExtractor(17-21)Task(23-30)Task(32-41)Task(43-53)Task(55-97)Task(99-112)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs (5)
FaviconWebClient(13-130)FaviconWebClient(19-31)Task(33-73)Task(75-123)Dispose(125-129)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/HtmlFaviconParser.cs (3)
HtmlFaviconParser(10-105)HtmlFaviconParser(25-28)List(30-81)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (6)
ImageConverter(13-129)ImageConverter(18-21)Task(23-44)PngData(46-67)PngData(69-102)PngData(104-128)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (6)
Task(107-118)Task(158-214)List(67-89)List(231-260)Result(91-105)Dispose(262-271)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ViewModels/CustomBrowserSettingViewModel.cs (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs (1)
CustomBrowser(5-49)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs (2)
BrowserType(29-49)BrowserDetector(9-50)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs (4)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (4)
IBookmarkLoader(152-165)IEnumerable(56-59)IEnumerable(61-106)IEnumerable(108-150)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (1)
IAsyncEnumerable(41-84)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/IBookmarkLoader.cs (1)
IAsyncEnumerable(10-10)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs (2)
BrowserDetector(9-50)IEnumerable(11-27)
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs
[warning] 70-70:
wal is not a recognized word. (unrecognized-spelling)
[warning] 67-67:
shm is not a recognized word. (unrecognized-spelling)
[warning] 67-67:
shm is not a recognized word. (unrecognized-spelling)
[warning] 66-66:
wal is not a recognized word. (unrecognized-spelling)
[warning] 66-66:
wal is not a recognized word. (unrecognized-spelling)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs
[warning] 142-142:
Cooldown is not a recognized word. (unrecognized-spelling)
⏰ 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 (10)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ViewModels/CustomBrowserSettingViewModel.cs (1)
86-110: Folder selection via WPF OpenFileDialog looks good; no disposal needed.This uses Microsoft.Win32.OpenFileDialog (not IDisposable) with CheckPathExists, which is sufficient. Prior note about disposing FolderBrowserDialog doesn’t apply here.
Based on learnings
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs (1)
22-27: TLS validation restored — thank you.Removal of DangerousAcceptAnyServerCertificateValidator fixes the security hole.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs (2)
22-30: Dialog flow for adding custom browsers looks good.Adds on OK without forcing a reload here; Main listens and reloads.
59-66: Verify SelectedCustomBrowser binding.Ensure XAML binds the list’s SelectedItem to SelectedCustomBrowser so Edit flows work after focus changes.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (1)
134-159: Cancellation-aware shared fetch is correct now.Awaiting the shared task with WaitAsync(token) preserves sharing while honoring cancellation.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs (3)
9-9: Default Unknown is appropriate for detection flow.Using Unknown as the initial state aligns with BrowserDetector’s contract and the view-model validation.
54-56: Verify localization for “Unknown”.Ensure “Unknown” is localized across plugin language resources to match existing Chromium/Firefox entries.
51-62: Confirm enum serialization mode.If enums are serialized numerically, reordering can break compatibility. If string-based serialization is already configured, the risk is mitigated.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs (1)
113-116: Good fix: removed unsafe GetString() in no-type branch.This addresses the prior exception risk noted in earlier review.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/CustomBrowserSetting.xaml.cs (1)
26-36: Good fix: respect CanExecute before Save on Enter.This resolves the earlier issue where invalid paths could be saved via Enter.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs
Outdated
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs (2)
45-61: Avoid O(n²) string allocations in the head-scan loop.Calling contentBuilder.ToString() on every iteration is expensive. Scan the buffer incrementally for "" instead.
Apply this diff:
- var contentBuilder = new StringBuilder(); - var buffer = new char[4096]; - int charsRead; - var totalCharsRead = 0; - const int maxHeadChars = 150 * 1024; - - while (!token.IsCancellationRequested && (charsRead = await reader.ReadAsync(buffer, 0, buffer.Length)) > 0 && totalCharsRead < maxHeadChars) - { - contentBuilder.Append(buffer, 0, charsRead); - totalCharsRead += charsRead; - - if (contentBuilder.ToString().Contains("</head>", StringComparison.OrdinalIgnoreCase)) - { - break; - } - } + var contentBuilder = new StringBuilder(); + var buffer = new char[4096]; + int charsRead; + var totalCharsRead = 0; + const int maxHeadChars = 150 * 1024; + ReadOnlySpan<char> pattern = "</head>".AsSpan(); + var matchIndex = 0; + var foundHead = false; + + while (!token.IsCancellationRequested && + (charsRead = await reader.ReadAsync(buffer, 0, buffer.Length)) > 0 && + totalCharsRead < maxHeadChars) + { + contentBuilder.Append(buffer, 0, charsRead); + totalCharsRead += charsRead; + + for (var i = 0; i < charsRead; i++) + { + var c = char.ToLowerInvariant(buffer[i]); + var p = char.ToLowerInvariant(pattern[matchIndex]); + if (c == p) + { + matchIndex++; + if (matchIndex == pattern.Length) + { + foundHead = true; + break; + } + } + else + { + matchIndex = c == char.ToLowerInvariant(pattern[0]) ? 1 : 0; + } + } + if (foundHead) break; + }
88-107: Pre-size MemoryStream to reduce allocations and honor Content-Length.Small win on large icons and avoids re-alloc churn.
Apply this diff:
- await using var contentStream = await response.Content.ReadAsStreamAsync(token); - var memoryStream = new MemoryStream(); + await using var contentStream = await response.Content.ReadAsStreamAsync(token); + var contentLength = response.Content.Headers.ContentLength; + var capacity = (int)Math.Min(contentLength.GetValueOrDefault(MaxFaviconBytes), MaxFaviconBytes); + var memoryStream = new MemoryStream(capacity > 0 ? capacity : 0);Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (2)
117-144: Deduplicate cleanup logic via shared helper.This CleanupTempFiles duplicates LocalFaviconExtractor’s method. Consider sharing a small utility to reduce duplication.
41-92: Stream results instead of buffering whole list (optional).Yield as you read to cut memory and surface results sooner; only fallback path needs a retry branch.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (2)
100-131: Guard against rare concurrent writes to the same cache file.Parallel processing could race on identical URLs. Use CreateNew and fall back if already written.
Apply this diff:
- await File.WriteAllBytesAsync(cachePath, pngData, token); - bookmark.FaviconPath = cachePath; + try + { + await using var fs = new FileStream(cachePath, FileMode.CreateNew, FileAccess.Write, FileShare.Read); + await fs.WriteAsync(pngData, token); + bookmark.FaviconPath = cachePath; + } + catch (IOException) + { + // Someone else created it; just use it. + if (File.Exists(cachePath)) + bookmark.FaviconPath = cachePath; + } return;
36-54: Add note: cache eviction is out of scope but advisable.Consider a size-/age-based cleanup (LRU by last access) capped to a few hundred MB to prevent unbounded growth.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (1)
44-47: Fire-and-forget without logging can hide failures (optional).Wrap background calls with a try/catch logger to avoid unobserved exceptions.
Also applies to: 135-139, 120-127
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs(2 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-06T05:26:32.163Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3664
File: Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs:326-326
Timestamp: 2025-06-06T05:26:32.163Z
Learning: In the Firefox bookmark loader (FirefoxBookmarkLoader.cs), using hardcoded Windows line endings ("\r\n") for splitting profiles.ini content is acceptable and platform-agnostic line splitting is not required.
Applied to files:
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs
🧬 Code graph analysis (4)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (7)
Task(75-92)Task(94-132)Task(134-159)Task(172-239)Task(255-276)Task(278-291)Dispose(293-301)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (1)
Task(23-44)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (8)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Settings(5-51)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (2)
BookmarkLoaderService(13-166)BookmarkLoaderService(19-24)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (9)
FaviconService(17-302)FaviconService(36-54)Task(75-92)Task(94-132)Task(134-159)Task(172-239)Task(255-276)Task(278-291)Dispose(293-301)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkWatcherService.cs (4)
BookmarkWatcherService(7-94)BookmarkWatcherService(17-20)UpdateWatchers(22-53)Dispose(76-93)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (1)
Task(94-115)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (5)
Task(23-38)Task(40-49)Task(51-61)Task(63-105)Task(107-120)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs (1)
BrowserType(29-49)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs (2)
SettingsControl(8-67)SettingsControl(13-17)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (6)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Settings(5-51)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (7)
LocalFaviconExtractor(12-150)LocalFaviconExtractor(17-21)Task(23-38)Task(40-49)Task(51-61)Task(63-105)Task(107-120)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs (5)
FaviconWebClient(13-130)FaviconWebClient(19-31)Task(33-73)Task(75-123)Dispose(125-129)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/HtmlFaviconParser.cs (3)
HtmlFaviconParser(10-105)HtmlFaviconParser(25-28)List(30-81)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (6)
ImageConverter(13-129)ImageConverter(18-21)Task(23-44)PngData(46-67)PngData(69-102)PngData(104-128)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (6)
Task(107-118)Task(158-214)List(67-89)List(231-260)Result(91-105)Dispose(262-271)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (3)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (2)
IBookmarkLoader(152-165)Task(26-54)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/IBookmarkLoader.cs (1)
IAsyncEnumerable(10-10)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (6)
CleanupTempFiles(122-149)Task(23-38)Task(40-49)Task(51-61)Task(63-105)Task(107-120)
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs
[warning] 142-142:
Cooldown is not a recognized word. (unrecognized-spelling)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs
[warning] 66-66:
shm is not a recognized word. (unrecognized-spelling)
[warning] 65-65:
wal is not a recognized word. (unrecognized-spelling)
[warning] 65-65:
wal is not a recognized word. (unrecognized-spelling)
[warning] 64-64:
wal is not a recognized word. (unrecognized-spelling)
[warning] 61-61:
shm is not a recognized word. (unrecognized-spelling)
[warning] 60-60:
wal is not a recognized word. (unrecognized-spelling)
⏰ 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 (6)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs (1)
22-31: TLS validation is now honored — good fix.The insecure ServerCertificateCustomValidationCallback override is gone. This resolves the prior MITM risk.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (1)
22-31: SQL join/filters look correct now.Using IS NOT NULL and guarding url aligns with SQLite syntax and avoids nulls.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (3)
134-159: Cancellation handling in shared web fetch is correct.Awaiting the shared task with WaitAsync(token) and not canceling the producer fixes prior cancellation gap.
161-170: Good: deterministic cache filenames via SHA-256.Stable hashing avoids path issues and collisions.
241-253: Best-result selection now respects non-null PngData.Fix addresses prior bug where a larger “empty” result could win.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (1)
67-85: Volatile.Read in Query — good.Prevents stale reads when swapping the list.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs
Outdated
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (1)
41-94: Consider optimizing the fallback logic to avoid unnecessary copies.Currently, the code always attempts a direct read first and falls back to copying if the database is locked. When Firefox is known to be running (which is common), this results in a predictable failure followed by file copying on every load.
Consider checking if Firefox is running before attempting the direct read to skip the failed attempt when unnecessary.
public async IAsyncEnumerable<Bookmark> GetBookmarksAsync([EnumeratorCancellation] CancellationToken cancellationToken = default) { if (string.IsNullOrEmpty(_placesPath) || !File.Exists(_placesPath)) yield break; var bookmarks = new List<Bookmark>(); string? tempDbPath = null; + bool firefoxIsRunning = IsFirefoxRunning(); // Helper to check if Firefox process is active try { - // First, try to read directly from the source to avoid a slow file copy - await ReadBookmarksFromDb(_placesPath, bookmarks, cancellationToken); + if (!firefoxIsRunning) + { + // Try to read directly from the source when Firefox is not running + await ReadBookmarksFromDb(_placesPath, bookmarks, cancellationToken); + } + else + { + // Firefox is running, go straight to fallback copy + throw new SqliteException("Firefox is running", 5); + } }This optimization would improve performance in the common case where Firefox is open by avoiding the predictable direct read failure.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (2)
100-107: Unify cache keying (URL vs origin) to avoid duplicate writes and extra work.You cache web results by origin but check/save local results by full URL. This causes redundant lookups/writes and inconsistent paths.
Apply this diff to prefer origin cache when available and write local conversions to the same path:
- var cachePath = GetCachePath(bookmark.Url, _faviconCacheDir); - if (File.Exists(cachePath)) - { - bookmark.FaviconPath = cachePath; - return; - } + var urlCachePath = GetCachePath(bookmark.Url, _faviconCacheDir); + string? originCachePath = null; + if (Uri.TryCreate(bookmark.Url, UriKind.Absolute, out var parsed)) + originCachePath = GetCachePath(parsed.GetLeftPart(UriPartial.Authority), _faviconCacheDir); + var existingPath = originCachePath != null && File.Exists(originCachePath) + ? originCachePath + : (File.Exists(urlCachePath) ? urlCachePath : null); + if (existingPath != null) + { + bookmark.FaviconPath = existingPath; + return; + } @@ - var (pngData, _) = await _imageConverter.ToPngAsync(new MemoryStream(localData), token); + var (pngData, _) = await _imageConverter.ToPngAsync(new MemoryStream(localData), token); if (pngData != null) { - await File.WriteAllBytesAsync(cachePath, pngData, token); - bookmark.FaviconPath = cachePath; + var destPath = originCachePath ?? urlCachePath; + await File.WriteAllBytesAsync(destPath, pngData, token); + bookmark.FaviconPath = destPath; return; }No change needed here; just noting the mismatch source:
- var cachePath = GetCachePath(urlString, _faviconCacheDir); + var cachePath = GetCachePath(urlString, _faviconCacheDir); // origin-basedAlso applies to: 113-119, 172-177
41-47: Future: add eviction for orphaned favicons.You create/cache indefinitely under FaviconCache but don’t prune. Consider periodic cleanup keyed to current bookmarks or LRU with cap.
I can draft a lightweight cleanup that deletes cache files not referenced by current bookmarks and older than N days. Want a patch?
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (2)
213-221: Avoid duplicate bookmarks on periodic Firefox reload.Use Distinct on the newly loaded Firefox set and the final concat to prevent dupes.
Apply this diff:
- var newFirefoxBookmarks = successfulResults.SelectMany(r => r.Bookmarks).ToList(); + var newFirefoxBookmarks = successfulResults.SelectMany(r => r.Bookmarks).Distinct().ToList(); @@ - var newBookmarkList = otherBookmarks.Concat(newFirefoxBookmarks).Distinct().ToList(); + var newBookmarkList = otherBookmarks.Concat(newFirefoxBookmarks).Distinct().ToList();
282-289: Unsubscribe the file-change handler on dispose.Prevents dangling delegate references and eases GC.
Apply this diff:
public void Dispose() { _settings.PropertyChanged -= OnSettingsPropertyChanged; _settings.CustomBrowsers.CollectionChanged -= OnCustomBrowsersChanged; + _bookmarkWatcher.OnBookmarkFileChanged -= OnBookmarkFileChanged; _firefoxBookmarkTimer?.Dispose(); _cancellationTokenSource.Cancel(); _cancellationTokenSource.Dispose(); _faviconService.Dispose(); _bookmarkWatcher.Dispose(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs(2 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-06T05:26:32.163Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3664
File: Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs:326-326
Timestamp: 2025-06-06T05:26:32.163Z
Learning: In the Firefox bookmark loader (FirefoxBookmarkLoader.cs), using hardcoded Windows line endings ("\r\n") for splitting profiles.ini content is acceptable and platform-agnostic line splitting is not required.
Applied to files:
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs
🧬 Code graph analysis (3)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (2)
IBookmarkLoader(152-165)Task(26-54)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (6)
CleanupTempFiles(122-149)Task(23-38)Task(40-49)Task(51-61)Task(63-105)Task(107-120)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (6)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Settings(5-51)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (7)
LocalFaviconExtractor(12-150)LocalFaviconExtractor(17-21)Task(23-38)Task(40-49)Task(51-61)Task(63-105)Task(107-120)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs (5)
FaviconWebClient(13-130)FaviconWebClient(19-31)Task(33-73)Task(75-123)Dispose(125-129)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/HtmlFaviconParser.cs (3)
HtmlFaviconParser(10-105)HtmlFaviconParser(25-28)List(30-81)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (6)
ImageConverter(13-129)ImageConverter(18-21)Task(23-44)PngData(46-67)PngData(69-102)PngData(104-128)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (5)
Task(108-127)Task(167-232)List(68-90)List(249-278)Dispose(280-289)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (7)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Settings(5-51)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (2)
BookmarkLoaderService(13-166)BookmarkLoaderService(19-24)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (9)
FaviconService(19-302)FaviconService(36-54)Task(75-92)Task(94-132)Task(134-159)Task(172-239)Task(255-276)Task(278-291)Dispose(293-301)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkWatcherService.cs (4)
BookmarkWatcherService(7-94)BookmarkWatcherService(17-20)UpdateWatchers(22-53)Dispose(76-93)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (1)
Task(96-117)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (5)
Task(23-38)Task(40-49)Task(51-61)Task(63-105)Task(107-120)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs (2)
SettingsControl(8-67)SettingsControl(13-17)
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs
[warning] 66-66:
shm is not a recognized word. (unrecognized-spelling)
[warning] 65-65:
wal is not a recognized word. (unrecognized-spelling)
[warning] 65-65:
wal is not a recognized word. (unrecognized-spelling)
[warning] 64-64:
wal is not a recognized word. (unrecognized-spelling)
[warning] 61-61:
shm is not a recognized word. (unrecognized-spelling)
[warning] 60-60:
wal is not a recognized word. (unrecognized-spelling)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs
[warning] 142-142:
Cooldown is not a recognized word. (unrecognized-spelling)
⏰ 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 (8)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (4)
22-31: Good job fixing the SQL syntax issue!The query now correctly uses
IS NOT NULLin the WHERE clause instead of the invalidNOT NULLsyntax in JOIN conditions. This ensures proper SQL execution and filtering of null values.
56-77: Good implementation of WAL/SHM support and duplicate prevention!The implementation correctly:
- Copies WAL/SHM files alongside the main database to preserve recent changes when Firefox is running
- Clears the bookmarks collection before fallback read to prevent duplicates
This ensures bookmark data consistency during the fallback scenario.
96-100: ProfilePath correctly uses the original Firefox profile directory.The implementation properly derives the profile path from
_placesPath(the original location) rather thandbPath(which could be a temp copy). This ensures favicon extraction works correctly even when reading from a temporary database copy.
119-146: CleanupTempFiles implementation is robust and matches LocalFaviconExtractor pattern.The cleanup method properly handles all associated database files (WAL/SHM) using a wildcard pattern and includes appropriate error handling. The implementation is consistent with the same pattern used in
LocalFaviconExtractor.cs.Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (3)
134-159: Cancellation honored correctly for shared fetch.Using WaitAsync(token) with try/catch cleanly detaches caller cancellation without killing the shared task. LGTM.
161-169: Good: secure, stable cache keys.Switched to SHA-256 for deterministic filenames. This addresses weak-hash feedback.
241-253: Fixed: selection now ignores null candidates.The selector prefers non-null PngData and size appropriately. Resolves the earlier drop of valid favicons.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (1)
71-71: Correct memory barriers for bookmark swap.Using Volatile.Read matches the Volatile.Write in reload and avoids stale reads.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (1)
41-47: Future: Cache eviction strategy.Consider size/time-based eviction for FaviconCache and periodic cleanup of FaviconFails.json entries older than the cooldown window.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (1)
144-147: Optional: Fire-and-forget error surfacing.Consider logging failures from ReloadDataAsync to avoid silent task faults.
Apply pattern:
- _ = ReloadDataAsync(); + _ = ReloadDataAsync().ContinueWith(t => Context.API.LogException(nameof(Main), "ReloadDataAsync failed.", t.Exception!), TaskContinuationOptions.OnlyOnFaulted);Also applies to: 45-48, 131-136, 140-142
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs(2 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (7)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Settings(5-51)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (2)
BookmarkLoaderService(13-166)BookmarkLoaderService(19-24)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (9)
FaviconService(19-305)FaviconService(36-54)Task(75-95)Task(97-135)Task(137-162)Task(175-242)Task(258-279)Task(281-294)Dispose(296-304)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkWatcherService.cs (4)
BookmarkWatcherService(7-94)BookmarkWatcherService(17-20)UpdateWatchers(22-53)Dispose(76-93)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (1)
Task(96-117)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (5)
Task(23-38)Task(40-49)Task(51-61)Task(63-105)Task(107-120)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs (2)
SettingsControl(8-67)SettingsControl(13-17)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (6)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Settings(5-51)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (7)
LocalFaviconExtractor(12-150)LocalFaviconExtractor(17-21)Task(23-38)Task(40-49)Task(51-61)Task(63-105)Task(107-120)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs (5)
FaviconWebClient(13-130)FaviconWebClient(19-31)Task(33-73)Task(75-123)Dispose(125-129)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/HtmlFaviconParser.cs (3)
HtmlFaviconParser(10-105)HtmlFaviconParser(25-28)List(30-81)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (6)
ImageConverter(13-129)ImageConverter(18-21)Task(23-44)PngData(46-67)PngData(69-102)PngData(104-128)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (6)
Task(108-127)Task(173-238)List(68-90)List(255-284)Result(92-106)Dispose(286-295)
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs
[warning] 145-145:
Cooldown is not a recognized word. (unrecognized-spelling)
⏰ 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 (9)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (5)
17-17: Good: FaviconCandidate moved to namespace scope.Unblocks unqualified references from other types (e.g., HtmlFaviconParser).
164-173: Good: SHA-256 for cache hashing.Avoids weak SHA-1 and ensures deterministic, collision-resistant filenames.
244-256: Good: Selection now ignores null PngData.Fixes prior bug dropping valid favicons.
50-54: Security: Ensure HttpClient uses default TLS validation.Prior review flagged DangerousAcceptAnyServerCertificateValidator in FaviconWebClient. Please confirm it’s removed.
#!/bin/bash # Search for insecure TLS overrides rg -nP -C2 '(DangerousAcceptAnyServerCertificateValidator|ServerCertificateCustomValidationCallback)' --type=cs
137-162: Approve code changes: Web fetch now honors cancellation via Task.WaitAsync(token), preventing stuck callers while preserving the shared fetch.Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (4)
68-90: Good: Volatile.Read paired with Volatile.Write.Prevents stale reads during atomic swaps in Query.
108-127: Good: Serialized reloads via _reloadGate.Avoids overlapping swaps and watcher races.
156-170: Good: Hardened PeriodicTimer loop.Local snapshot and exception handling prevent disposal races.
219-233: Good: Targeted Firefox reload without blocking full list.Merges results by source and kicks off favicon processing for just the new set.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (2)
124-126: Minor: Dispose the transient MemoryStream.Use a using-var to avoid a tiny allocation leak and signal intent.
Apply this diff:
- var (pngData, _) = await _imageConverter.ToPngAsync(new MemoryStream(localData), token); + using var ms = new MemoryStream(localData); + var (pngData, _) = await _imageConverter.ToPngAsync(ms, token);
320-329: Dispose owned components if they implement IDisposable._localExtractor, _htmlParser, and _imageConverter may hold unmanaged resources. Dispose them if applicable.
Consider:
public void Dispose() { _cts.Cancel(); _ongoingFetches.Clear(); (_localExtractor as IDisposable)?.Dispose(); (_htmlParser as IDisposable)?.Dispose(); (_imageConverter as IDisposable)?.Dispose(); _webClient.Dispose(); _fileLock.Dispose(); _cts.Dispose(); GC.SuppressFinalize(this); }Please confirm whether these types implement IDisposable. If not, ignore.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs
[warning] 162-162:
Cooldown is not a recognized word. (unrecognized-spelling)
⏰ 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 (7)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (7)
17-17: Resolved: FaviconCandidate promoted to namespace scope.This addresses the earlier compile-visibility concern. Looks good.
75-96: Resolved: SaveFailedFetchesAsync now swallows ObjectDisposedException.The added ODE catch prevents shutdown-time task faults. File lock handling is correct.
104-120: Unified host/page cache key checks are correct.Preferring a host-level cache while still honoring preexisting page-level files avoids duplication and improves hit rate.
154-179: Resolved: Web fetch honors cancellation without canceling the shared task.Using WaitAsync(token) with OCE handling matches the intended behavior.
181-190: Good: Cache path uses SHA‑256.Secure, collision‑resistant hashing for filenames addresses prior SHA1 concern.
268-280: Resolved: Selection guards against null PngData.Selection logic now prefers available data and avoids dropping valid results.
50-54: FaviconWebClient enforces default TLS certificate validation.
No custom ServerCertificateCustomValidationCallback is set; HttpClientHandler uses strict validation by default.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (5)
106-142: Avoid races and small memory leak in local-cache write path
- The MemoryStream instance isn’t disposed.
- Writes can still race; prefer atomic temp-write + move.
Apply this diff:
- var (pngData, _) = await _imageConverter.ToPngAsync(new MemoryStream(localData), token); + using var ms = new MemoryStream(localData, writable: false); + var (pngData, _) = await _imageConverter.ToPngAsync(ms, token); if (pngData != null) { - var path = hostCachePath; - try - { - await File.WriteAllBytesAsync(path, pngData, token); - } - catch (IOException) - { - // Another thread may have created it concurrently. - } + var path = hostCachePath; + var tmp = path + "." + Guid.NewGuid().ToString("N") + ".tmp"; + try + { + await File.WriteAllBytesAsync(tmp, pngData, token); + try { File.Move(tmp, path, overwrite: false); } + catch (IOException) + { + // Another thread may have created it concurrently. + } + } + finally + { + try { if (File.Exists(tmp)) File.Delete(tmp); } catch { /* best effort */ } + } if (File.Exists(path)) { bookmark.FaviconPath = path; return; } // If write failed and file still doesn't exist, fall through to web fallback. }
98-105: Parallelism constant: consider making it configurable or CPU-awareMaxDegreeOfParallelism = 8 is reasonable, but making it configurable or using Environment.ProcessorCount could better fit diverse machines.
196-253: Make web-cache writes atomic to eliminate race and partial-file risksCurrent direct writes may race; prefer temp-write + atomic move, mirroring the local path suggestion.
Apply this diff:
- if (bestResult.PngData != null) + if (bestResult.PngData != null) { try { - await File.WriteAllBytesAsync(cachePath, bestResult.PngData, _cts.Token); + var tmp = cachePath + "." + Guid.NewGuid().ToString("N") + ".tmp"; + try + { + await File.WriteAllBytesAsync(tmp, bestResult.PngData, _cts.Token); + try { File.Move(tmp, cachePath, overwrite: false); } + catch (IOException) + { + // Another thread may have created it concurrently. + } + } + finally + { + try { if (File.Exists(tmp)) File.Delete(tmp); } catch { /* best effort */ } + } } catch (IOException) { // Another thread may have created it concurrently. } if (File.Exists(cachePath)) { _context.API.LogDebug(nameof(FaviconService), $"Favicon for {urlString} cached successfully."); if (_failedFetches.TryRemove(urlString, out _)) { _ = SaveFailedFetchesAsync(); } return cachePath; } // Do not treat as success; let finally record failure if needed. }
266-271: Optional: prune aged failure entries on load/saveTo keep FaviconFails.json small, consider dropping entries older than FailedFaviconCooldown during LoadFailedFetches or just before Save.
328-336: Optional: best‑effort flush before cancel on disposeIf you want to persist last updates, consider a best‑effort SaveFailedFetchesAsync using CancellationToken.None before calling _cts.Cancel().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (6)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Settings(5-51)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (7)
LocalFaviconExtractor(12-150)LocalFaviconExtractor(17-21)Task(23-38)Task(40-49)Task(51-61)Task(63-105)Task(107-120)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs (5)
FaviconWebClient(13-130)FaviconWebClient(19-31)Task(33-73)Task(75-123)Dispose(125-129)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/HtmlFaviconParser.cs (3)
HtmlFaviconParser(10-105)HtmlFaviconParser(25-28)List(30-81)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (6)
ImageConverter(13-129)ImageConverter(18-21)Task(23-44)PngData(46-67)PngData(69-102)PngData(104-128)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (6)
Task(108-127)Task(173-238)List(68-90)List(255-284)Result(92-106)Dispose(286-295)
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs
[warning] 166-166:
Cooldown is not a recognized word. (unrecognized-spelling)
⏰ 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 (5)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (5)
17-17: FaviconCandidate promoted to namespace scope — LGTMThis resolves the earlier visibility/qualification issue with HtmlFaviconParser.
75-96: Graceful shutdown: SaveFailedFetchesAsync handles ODE — LGTMCatching ObjectDisposedException (and OCE) avoids unobserved task faults during shutdown.
158-183: Cancellation honored for shared web fetch — LGTMUsing WaitAsync(token) with per-caller cancellation while keeping the shared fetch alive addresses the earlier cancellation gap.
185-194: Secure hashing for cache keys — LGTMSHA-256 hashing for deterministic cache paths addresses the weak-hash concern.
276-288: Best-result selection fixed to avoid null PngData — LGTMNull checks before size comparison prevent dropping valid favicons.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (2)
172-178: Consider "cooldown" spelling in commentsThe static analysis tool flags "Cooldown" as unrecognized. While this is commonly used in technical contexts, consider using "cool-down period" or "backoff period" for formal documentation if style guidelines require it.
98-163: Extract MaxDegreeOfParallelism into a configurable setting
Move the hard-codedMaxDegreeOfParallelism = 8in FaviconService.cs (line 102) into a newSettings.MaxFaviconParallelismproperty (default 8) so users can tune concurrency for I/O-heavy favicon fetching.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (6)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Settings(5-51)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (7)
LocalFaviconExtractor(12-150)LocalFaviconExtractor(17-21)Task(23-38)Task(40-49)Task(51-61)Task(63-105)Task(107-120)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs (5)
FaviconWebClient(13-130)FaviconWebClient(19-31)Task(33-73)Task(75-123)Dispose(125-129)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/HtmlFaviconParser.cs (3)
HtmlFaviconParser(10-105)HtmlFaviconParser(25-28)List(30-81)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (6)
ImageConverter(13-129)ImageConverter(18-21)Task(23-44)PngData(46-67)PngData(69-102)PngData(104-128)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (5)
Task(108-127)Task(173-238)List(68-90)List(255-284)Dispose(286-295)
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs
[warning] 173-173:
Cooldown is not a recognized word. (unrecognized-spelling)
⏰ 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 (8)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (8)
17-17: Move FaviconCandidate to namespace scopeHtmlFaviconParser references it unqualified, but it's currently nested inside FaviconService.
Based on previous review, this was already flagged. The FaviconCandidate is now at the correct namespace scope, so this is resolved.
56-73: LGTM!The LoadFailedFetches method correctly loads the failed fetches from JSON and handles exceptions appropriately. The use of case-insensitive StringComparer is good for URL authorities.
75-96: LGTM!Good fix - the method now properly handles ObjectDisposedException during disposal races, as recommended in previous reviews.
194-201: LGTM: Using SHA256 provides better security than SHA1The code correctly uses SHA256.HashData() for generating cache keys, which is the recommended choice over SHA-1 due to security considerations. Microsoft recommends SHA-256 or better due to collision problems with SHA-1. The static HashData method is also more efficient than creating and managing a HashAlgorithm instance.
104-163: Improved cache handling with atomic writesThe ProcessBookmarkFavicons method now implements the cache key unification and atomic writes that were suggested in previous reviews. The logic correctly:
- Prefers host-based cache paths for consistency
- Falls back to page-based cache paths for legacy compatibility
- Uses atomic temp file writes with proper cleanup
- Only sets FaviconPath when the file actually exists
165-190: LGTM: Proper cancellation handling in web fetchThe GetFaviconFromWebAsync method correctly implements cancellation handling as suggested in previous reviews. It now honors the cancellation token via WaitAsync() while keeping the shared fetch task alive for other callers.
240-280: LGTM: Fixed cache existence verificationThe FetchAndCacheFaviconAsync method correctly implements the fixes from previous reviews:
- Only logs success and returns cache path if the file actually exists
- Proper error handling for concurrent writes
- Failure tracking only occurs when fetch was attempted and no cache file exists
283-295: LGTM: Fixed favicon selection logicThe SelectBestFavicon method correctly implements the null check fixes from previous reviews, ensuring that results with null PngData are not selected over valid alternatives.
…/Flow.Launcher into BrowserBookmark-Advanced
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs (1)
CustomBrowser(5-49)
⏰ 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
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated 6 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| namespace Flow.Launcher.Plugin.BrowserBookmark.Services; | ||
|
|
||
| public readonly record struct FaviconCandidate(string Url, int Score); |
There was a problem hiding this comment.
[nitpick] The FaviconCandidate record is defined inside the service file but is used by other services like HtmlFaviconParser. Consider moving this to a separate Models file or namespace to improve code organization and reusability.
Copilot uses AI. Check for mistakes.
| [GeneratedRegex("<link[^>]+?>", RegexOptions.IgnoreCase | RegexOptions.Compiled, "en-US")] | ||
| private static partial Regex LinkTagRegex(); |
There was a problem hiding this comment.
Using multiple compiled regexes can impact startup performance. Consider combining related patterns or using a single regex with named groups to reduce the compilation overhead during service initialization.
Copilot uses AI. Check for mistakes.
| { | ||
| if (!_settings.EnableFavicons) return; | ||
|
|
||
| var options = new ParallelOptions { MaxDegreeOfParallelism = 8, CancellationToken = cancellationToken }; |
There was a problem hiding this comment.
[nitpick] The MaxDegreeOfParallelism value of 8 is hardcoded. Consider making this configurable through settings or determining it dynamically based on system capabilities (e.g., Environment.ProcessorCount) to optimize performance across different hardware configurations.
| var options = new ParallelOptions { MaxDegreeOfParallelism = 8, CancellationToken = cancellationToken }; | |
| var options = new ParallelOptions { MaxDegreeOfParallelism = Environment.ProcessorCount, CancellationToken = cancellationToken }; |
Copilot uses AI. Check for mistakes.
| private List<Bookmark> _bookmarks = []; | ||
| private readonly CancellationTokenSource _cancellationTokenSource = new(); | ||
| private PeriodicTimer? _firefoxBookmarkTimer; | ||
| private static readonly TimeSpan FirefoxPollingInterval = TimeSpan.FromHours(3); |
There was a problem hiding this comment.
[nitpick] The Firefox polling interval is hardcoded to 3 hours. Consider making this configurable through settings to allow users to adjust the refresh frequency based on their usage patterns.
| private static readonly TimeSpan FirefoxPollingInterval = TimeSpan.FromHours(3); | |
| private TimeSpan FirefoxPollingInterval => TimeSpan.FromHours(_settings.FirefoxPollingIntervalHours); |
Copilot uses AI. Check for mistakes.
| lock (_lock) | ||
| { | ||
| if (_disposed) return; | ||
| _debounceTimer?.Change(2000, Timeout.Infinite); |
There was a problem hiding this comment.
[nitpick] The debounce delay of 2000ms is hardcoded. Consider making this configurable or define it as a named constant to improve maintainability and allow for future adjustments.
Copilot uses AI. Check for mistakes.
| { | ||
| private readonly HttpClient _httpClient; | ||
| private readonly PluginInitContext _context; | ||
| private const int MaxFaviconBytes = 250 * 1024; |
There was a problem hiding this comment.
[nitpick] Consider making the MaxFaviconBytes limit configurable through settings. Some high-quality favicons may exceed 250KB, and users might want to adjust this limit based on their preferences and bandwidth considerations.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (17)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs (2)
50-51: Open with broader FileShare to read while the browser holds the file.Chromium may keep Bookmarks open/locked. Use ReadWrite|Delete sharing to reduce IO errors and retries.
- await using var stream = File.OpenRead(bookmarkPath); + await using var stream = new FileStream( + bookmarkPath, FileMode.Open, FileAccess.Read, + FileShare.ReadWrite | FileShare.Delete);
47-57: Stream results directly; drop the intermediate List allocation.Yield directly from the JSON traversal to lower memory and start producing results earlier.
- var bookmarks = new List<Bookmark>(); try { await using var stream = File.OpenRead(bookmarkPath); using var jsonDocument = await JsonDocument.ParseAsync(stream, cancellationToken: cancellationToken); if (jsonDocument.RootElement.TryGetProperty("roots", out var rootElement)) { - bookmarks.AddRange(EnumerateBookmarks(rootElement, source, profilePath)); + foreach (var b in EnumerateBookmarks(rootElement, source, profilePath)) + yield return b; } } @@ - foreach (var bookmark in bookmarks) - { - yield return bookmark; - }And convert the helpers to iterators:
- private IEnumerable<Bookmark> EnumerateBookmarks(JsonElement rootElement, string source, string profilePath) - { - var bookmarks = new List<Bookmark>(); - foreach (var folder in rootElement.EnumerateObject()) - { - if (folder.Value.ValueKind != JsonValueKind.Object) - continue; - - // Fix for Opera. It stores bookmarks slightly different than chrome. - if (folder.Name == "custom_root") - bookmarks.AddRange(EnumerateBookmarks(folder.Value, source, profilePath)); - else - EnumerateFolderBookmark(folder.Value, bookmarks, source, profilePath); - } - return bookmarks; - } + private IEnumerable<Bookmark> EnumerateBookmarks(JsonElement rootElement, string source, string profilePath) + { + foreach (var folder in rootElement.EnumerateObject()) + { + if (folder.Value.ValueKind != JsonValueKind.Object) + continue; + + if (folder.Name == "custom_root") + { + foreach (var b in EnumerateBookmarks(folder.Value, source, profilePath)) + yield return b; + } + else + { + foreach (var b in EnumerateFolderBookmark(folder.Value, source, profilePath)) + yield return b; + } + } + } @@ - private void EnumerateFolderBookmark(JsonElement folderElement, ICollection<Bookmark> bookmarks, string source, string profilePath) + private IEnumerable<Bookmark> EnumerateFolderBookmark(JsonElement folderElement, string source, string profilePath) { if (!folderElement.TryGetProperty("children", out var childrenElement)) - return; + yield break; foreach (var subElement in childrenElement.EnumerateArray()) { - if (subElement.TryGetProperty("type", out var type)) + if (subElement.TryGetProperty("type", out var type)) { switch (type.GetString()) { case "folder": case "workspace": // Edge Workspace - EnumerateFolderBookmark(subElement, bookmarks, source, profilePath); + foreach (var b in EnumerateFolderBookmark(subElement, source, profilePath)) + yield return b; break; case "url": if (subElement.TryGetProperty("name", out var name) && subElement.TryGetProperty("url", out var url) && !string.IsNullOrEmpty(name.GetString()) && !string.IsNullOrEmpty(url.GetString())) { - bookmarks.Add(new Bookmark(name.GetString()!, url.GetString()!, source, profilePath)); + yield return new Bookmark(name.GetString()!, url.GetString()!, source, profilePath); } break; } } else { _logException(nameof(ChromiumBookmarkLoader), "type property not found in bookmark node.", null); } } }Note: If you also apply the ValueKind guards above, mirror them here.
Also applies to: 75-79, 82-97, 99-130
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs (1)
16-17: Guard directory enumeration and use case-insensitive Distinct on Windows.Avoid rare UnauthorizedAccess/IO issues and normalize duplicates on case-insensitive filesystems.
- var profileDirs = Directory.EnumerateDirectories(basePath, "Profile *").ToList(); + List<string> profileDirs; + try + { + profileDirs = Directory.EnumerateDirectories(basePath, "Profile *").ToList(); + } + catch + { + // Fall back to base path only. + return new[] { basePath }; + } @@ - return profileDirs.Distinct(); + return profileDirs.Distinct(StringComparer.OrdinalIgnoreCase);Also applies to: 26-26
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkWatcherService.cs (2)
24-31: Detach handlers before disposing old watchers.Avoid lingering delegates and reduce GC pressure.
- foreach (var watcher in _watchers) - { - watcher.EnableRaisingEvents = false; - watcher.Dispose(); - } + foreach (var watcher in _watchers) + { + watcher.EnableRaisingEvents = false; + watcher.Changed -= OnFileChanged; + watcher.Created -= OnFileChanged; + watcher.Deleted -= OnFileChanged; + watcher.Renamed -= OnFileRenamed; + watcher.Dispose(); + }
22-53: Synchronize mutations of _watchers.Dispose/update can race if called from different threads during shutdown. Use a dedicated lock.
private readonly object _lock = new(); + private readonly object _watchersLock = new(); @@ - public void UpdateWatchers(IEnumerable<string> filePaths) + public void UpdateWatchers(IEnumerable<string> filePaths) { - // Dispose old watchers - foreach (var watcher in _watchers) - { - watcher.EnableRaisingEvents = false; - watcher.Dispose(); - } - _watchers.Clear(); + lock (_watchersLock) + { + // Dispose old watchers + foreach (var watcher in _watchers) + { + watcher.EnableRaisingEvents = false; + watcher.Changed -= OnFileChanged; + watcher.Created -= OnFileChanged; + watcher.Deleted -= OnFileChanged; + watcher.Renamed -= OnFileRenamed; + watcher.Dispose(); + } + _watchers.Clear(); + } @@ - _watchers.Add(watcher); + lock (_watchersLock) + { + _watchers.Add(watcher); + } } } @@ - foreach (var watcher in _watchers) - { - watcher.EnableRaisingEvents = false; - watcher.Changed -= OnFileChanged; - watcher.Created -= OnFileChanged; - watcher.Deleted -= OnFileChanged; - watcher.Renamed -= OnFileRenamed; - watcher.Dispose(); - } + lock (_watchersLock) + { + foreach (var watcher in _watchers) + { + watcher.EnableRaisingEvents = false; + watcher.Changed -= OnFileChanged; + watcher.Created -= OnFileChanged; + watcher.Deleted -= OnFileChanged; + watcher.Renamed -= OnFileRenamed; + watcher.Dispose(); + } + _watchers.Clear(); + }Also applies to: 76-93
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ViewModels/CustomBrowserSettingViewModel.cs (1)
41-52: Unsubscribe from EditableBrowser.PropertyChanged on close.Avoid retaining the VM via the event if a consumer mistakenly holds the dialog open or reuses the VM.
public CustomBrowserSettingViewModel(CustomBrowser browser, Action<bool> closeAction) { @@ EditableBrowser.PropertyChanged += EditableBrowser_PropertyChanged; DetectEngineType(); } @@ private void Save() { _originalBrowser.Name = EditableBrowser.Name; _originalBrowser.DataDirectoryPath = EditableBrowser.DataDirectoryPath; _originalBrowser.BrowserType = EditableBrowser.BrowserType; + EditableBrowser.PropertyChanged -= EditableBrowser_PropertyChanged; _closeAction(true); } @@ private void Cancel() { + EditableBrowser.PropertyChanged -= EditableBrowser_PropertyChanged; _closeAction(false); }Also applies to: 70-77, 79-83
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (3)
22-31: Filter to bookmark rows only (type = 1).Prevents folders/separators/queries slipping through and reduces post‑processing.
INNER JOIN moz_bookmarks ON moz_bookmarks.fk = moz_places.id - WHERE moz_bookmarks.fk IS NOT NULL + WHERE moz_bookmarks.fk IS NOT NULL AND moz_bookmarks.title IS NOT NULL AND moz_places.url IS NOT NULL + AND moz_bookmarks.type = 1 ORDER BY moz_places.visit_count DESC
41-94: Consider streaming results to reduce memory and latency.Yield as you read to avoid building an intermediate list, keeping the fallback path intact by restricting the first attempt to a local iterator.
I can provide a minimal refactor that preserves the fallback semantics while yielding incrementally.
96-117: Bookmark equality excludes Source/ProfilePath
TheBookmarkrecord overridesEquals/GetHashCodeto use onlyNameandUrl, so.Distinct()will collapse entries from different browsers or profiles. If you need to treat bookmarks from separateSource(orProfilePath) as distinct, include those fields in the equality implementation or supply a dedicatedIEqualityComparer.Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/HtmlFaviconParser.cs (2)
56-57: Use class name as log tag for consistency.Aligns with other services.
- _context.API.LogDebug(nameof(Parse), $"Found potential favicon link. Raw tag: '{linkTag}', Extracted href: '{href}', Base URI: '{effectiveBaseUri}'"); + _context.API.LogDebug(nameof(HtmlFaviconParser), $"Found potential favicon link. Raw tag: '{linkTag}', Extracted href: '{href}', Base URI: '{effectiveBaseUri}'"); @@ - _context.API.LogDebug(nameof(Parse), $"Found suitable favicon candidate (score: {score}). Halting further HTML parsing."); + _context.API.LogDebug(nameof(HtmlFaviconParser), $"Found suitable favicon candidate (score: {score}). Halting further HTML parsing.");Also applies to: 74-75
87-98: Parse all sizes and pick the largest, not the first.Rel values often include multiple sizes; choosing the max improves icon quality.
- var sizesMatch = SizesAttributeRegex().Match(linkTag); - if (sizesMatch.Success) - { - var sizesValue = sizesMatch.Groups["v"].Value.ToUpperInvariant(); - if (sizesValue == "ANY") return 1000; - - var firstSizePart = sizesValue.Split(' ')[0]; - if (int.TryParse(firstSizePart.Split('X')[0], out var size)) - { - return size; - } - } + var sizesMatch = SizesAttributeRegex().Match(linkTag); + if (sizesMatch.Success) + { + var sizesValue = sizesMatch.Groups["v"].Value.ToUpperInvariant(); + if (sizesValue == "ANY") return 1000; + + var maxSide = 0; + foreach (var token in sizesValue.Split(new[] { ' ', ',', ';' }, StringSplitOptions.RemoveEmptyEntries)) + { + var xIdx = token.IndexOf('X'); + if (xIdx > 0 && int.TryParse(token.AsSpan(0, xIdx), out var side)) + maxSide = Math.Max(maxSide, side); + } + if (maxSide > 0) return maxSide; + }Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (3)
41-54: Propagate cancellation after aggregation to avoid partial swaps on shutdown.Currently each loader swallows OperationCanceledException; add a token check after Task.WhenAll to surface cancellation to callers (so Main can skip watcher updates/favicons).
await Task.WhenAll(tasks); + cancellationToken.ThrowIfCancellationRequested(); return (bookmarks.Distinct().ToList(), discoveredBookmarkFiles.Distinct().ToList());
99-106: Skip creating Firefox loaders when places.sqlite isn’t found.Avoid yielding loaders that will immediately yield-break by returning null from the factory and filtering at the call site.
- foreach (var browser in _settings.CustomBrowsers.Where(b => b.BrowserType == BrowserType.Firefox)) + foreach (var browser in _settings.CustomBrowsers.Where(b => b.BrowserType == BrowserType.Firefox)) { if (string.IsNullOrEmpty(browser.Name) || string.IsNullOrEmpty(browser.DataDirectoryPath) || !Directory.Exists(browser.DataDirectoryPath)) continue; - yield return CreateCustomFirefoxLoader(browser.Name, browser.DataDirectoryPath); + var loader = CreateCustomFirefoxLoader(browser.Name, browser.DataDirectoryPath); + if (loader != null) + yield return loader; } @@ - private IBookmarkLoader CreateCustomFirefoxLoader(string name, string dataDirectoryPath) + private IBookmarkLoader? CreateCustomFirefoxLoader(string name, string dataDirectoryPath) { var logAction = (string tag, string msg, Exception? ex) => _context.API.LogException(tag, msg, ex); // Custom Firefox paths might point to the root profile dir (e.g. ...\Mozilla\Firefox) var placesPath = FirefoxProfileFinder.GetPlacesPathFromProfileDir(dataDirectoryPath); if (string.IsNullOrEmpty(placesPath)) { // Or they might point directly to a profile folder (e.g. ...\Profiles\xyz.default-release) placesPath = Path.Combine(dataDirectoryPath, "places.sqlite"); } - // Do not add Firefox places.sqlite to the watcher as it's updated constantly for history. - return new FirefoxBookmarkLoader(name, placesPath, _tempPath, logAction); + if (string.IsNullOrEmpty(placesPath) || !File.Exists(placesPath)) + { + _context.API.LogWarn(nameof(BookmarkLoaderService), $"No places.sqlite under {dataDirectoryPath} for '{name}', skipping."); + return null; + } + // Do not add Firefox places.sqlite to the watcher as it's updated constantly for history. + return new FirefoxBookmarkLoader(name, placesPath, _tempPath, logAction); }Also applies to: 152-165
66-97: Optional: Add default detection for other Chromium browsers (Brave/Opera).Consider adding Brave and Opera default paths to improve OOTB coverage; custom browsers are a fallback.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (1)
113-122: Guard side-effects if cancellation was requested mid‑reload.Avoid updating watchers and starting favicon work during shutdown by bailing out after load completes.
var (bookmarks, discoveredFiles) = await _bookmarkLoader.LoadBookmarksAsync(_cancellation_tokenSource.Token); + if (_cancellationTokenSource.IsCancellationRequested) + return; // Atomically swap the list. This prevents the Query method from seeing a partially loaded list. Volatile.Write(ref _bookmarks, bookmarks);Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (2)
192-201: Micro-opt: Faster hex formatting.Use Convert.ToHexString to reduce allocations and simplify code.
- var hash = SHA256.HashData(Encoding.UTF8.GetBytes(url)); - var sb = new StringBuilder(hash.Length * 2); - foreach (byte b in hash) - { - sb.Append(b.ToString("x2")); - } - return Path.Combine(cacheDir, sb.ToString() + ".png"); + var hash = SHA256.HashData(Encoding.UTF8.GetBytes(url)); + var hex = Convert.ToHexString(hash).ToLowerInvariant(); + return Path.Combine(cacheDir, hex + ".png");
335-343: Offer: Add optional cache eviction for orphaned favicons.On full reload, compute live host keys from current bookmarks and delete stale files older than N days from FaviconCache. Happy to draft a small CleanupCacheAsync(bookmarks) for this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs(2 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkWatcherService.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxProfileFinder.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/HtmlFaviconParser.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/IBookmarkLoader.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/ViewModels/CustomBrowserSettingViewModel.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/CustomBrowserSetting.xaml.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxProfileFinder.cs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-06T05:26:32.163Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3664
File: Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs:326-326
Timestamp: 2025-06-06T05:26:32.163Z
Learning: In the Firefox bookmark loader (FirefoxBookmarkLoader.cs), using hardcoded Windows line endings ("\r\n") for splitting profiles.ini content is acceptable and platform-agnostic line splitting is not required.
Applied to files:
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs
📚 Learning: 2024-11-03T07:34:24.926Z
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:34:24.926Z
Learning: In Windows Forms dialogs, Windows handles invalid paths and prevents the user from clicking "Ok" if the path is incorrect, so additional path validation is unnecessary.
Applied to files:
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ViewModels/CustomBrowserSettingViewModel.cs
📚 Learning: 2024-11-03T07:40:11.014Z
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
Applied to files:
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ViewModels/CustomBrowserSettingViewModel.cs
🧬 Code graph analysis (14)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (3)
IEnumerable(56-59)IEnumerable(61-106)IEnumerable(108-150)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxProfileFinder.cs (2)
FirefoxProfileFinder(9-114)GetPlacesPathFromProfileDir(42-113)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (3)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (2)
IBookmarkLoader(152-165)Task(26-54)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/IBookmarkLoader.cs (1)
IAsyncEnumerable(10-10)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (6)
CleanupTempFiles(122-149)Task(23-38)Task(40-49)Task(51-61)Task(63-105)Task(107-120)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs (3)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (4)
IBookmarkLoader(152-165)IEnumerable(56-59)IEnumerable(61-106)IEnumerable(108-150)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/IBookmarkLoader.cs (1)
IAsyncEnumerable(10-10)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs (2)
BrowserDetector(9-50)IEnumerable(11-27)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (6)
Task(75-96)Task(98-163)Task(165-190)Task(203-281)Task(297-318)Task(320-333)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs (1)
CustomBrowser(5-49)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/CustomBrowserSetting.xaml.cs (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ViewModels/CustomBrowserSettingViewModel.cs (2)
CustomBrowserSettingViewModel(12-108)CustomBrowserSettingViewModel(41-52)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs (1)
CustomBrowser(5-49)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/HtmlFaviconParser.cs (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (2)
ImageConverter(13-129)ImageConverter(18-21)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkWatcherService.cs (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (2)
OnBookmarkFileChanged(144-147)Dispose(286-295)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/IBookmarkLoader.cs (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (1)
IBookmarkLoader(152-165)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ViewModels/CustomBrowserSettingViewModel.cs (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/CustomBrowser.cs (1)
CustomBrowser(5-49)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs (2)
BrowserType(29-49)BrowserDetector(9-50)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (6)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Settings(5-106)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (7)
LocalFaviconExtractor(12-150)LocalFaviconExtractor(17-21)Task(23-38)Task(40-49)Task(51-61)Task(63-105)Task(107-120)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs (5)
FaviconWebClient(13-130)FaviconWebClient(19-31)Task(33-73)Task(75-123)Dispose(125-129)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/HtmlFaviconParser.cs (3)
HtmlFaviconParser(9-104)HtmlFaviconParser(24-27)List(29-80)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (6)
ImageConverter(13-129)ImageConverter(18-21)Task(23-44)PngData(46-67)PngData(69-102)PngData(104-128)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (6)
Task(108-127)Task(173-238)List(68-90)List(255-284)Result(92-106)Dispose(286-295)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (3)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (6)
Task(75-96)Task(98-163)Task(165-190)Task(203-281)Task(297-318)Task(320-333)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (2)
Task(96-117)CleanupTempFiles(119-146)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (1)
Task(23-44)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (7)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Settings(5-106)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (3)
BookmarkLoaderService(13-166)BookmarkLoaderService(19-24)Task(26-54)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (9)
FaviconService(19-344)FaviconService(36-54)Task(75-96)Task(98-163)Task(165-190)Task(203-281)Task(297-318)Task(320-333)Dispose(335-343)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkWatcherService.cs (4)
BookmarkWatcherService(7-94)BookmarkWatcherService(17-20)UpdateWatchers(22-53)Dispose(76-93)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (1)
Task(96-117)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (5)
Task(23-38)Task(40-49)Task(51-61)Task(63-105)Task(107-120)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs (2)
SettingsControl(8-67)SettingsControl(13-17)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (6)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Settings(5-106)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (4)
Task(108-127)Task(173-238)List(68-90)List(255-284)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (3)
Task(96-117)FirefoxBookmarkLoader(13-147)FirefoxBookmarkLoader(33-39)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs (2)
IEnumerable(11-27)BrowserType(29-49)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs (3)
IEnumerable(82-97)ChromiumBookmarkLoader(13-131)ChromiumBookmarkLoader(22-28)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxProfileFinder.cs (4)
FirefoxProfileFinder(9-114)GetFirefoxPlacesPath(11-18)GetFirefoxMsixPlacesPath(20-40)GetPlacesPathFromProfileDir(42-113)
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs
[warning] 66-66:
shm is not a recognized word. (unrecognized-spelling)
[warning] 65-65:
wal is not a recognized word. (unrecognized-spelling)
[warning] 65-65:
wal is not a recognized word. (unrecognized-spelling)
[warning] 64-64:
wal is not a recognized word. (unrecognized-spelling)
[warning] 61-61:
shm is not a recognized word. (unrecognized-spelling)
[warning] 60-60:
wal is not a recognized word. (unrecognized-spelling)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs
[warning] 173-173:
Cooldown is not a recognized word. (unrecognized-spelling)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs
[warning] 139-139:
msix is not a recognized word. (unrecognized-spelling)
[warning] 137-137:
msix is not a recognized word. (unrecognized-spelling)
[warning] 131-131:
msix is not a recognized word. (unrecognized-spelling)
⏰ 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 (14)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs (3)
58-69: Good: IO and access errors are now handled.Adding IOException/UnauthorizedAccessException (plus generic) resolves the earlier gap when opening/reading the Bookmarks file.
125-128: Good: removed unsafe GetString() usage in the no-type branch.The log message no longer calls GetString() on a non-string JsonElement.
44-45: No action needed: discovered files are deduplicated downstream
TheBookmarkLoaderService.LoadBookmarksAsynccall applies.Distinct().ToList()todiscoveredBookmarkFilesbefore returning them, so duplicates cannot propagate into the watcher service.Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/IBookmarkLoader.cs (1)
7-11: Interface looks right for async, cancellable streaming.Contract is minimal and sufficient.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs (1)
29-49: LGTM overall.Browser type detection logic is clear and conservative.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (2)
14-90: Property‑changed pattern looks good.Backed fields with change guards and OnPropertyChanged across flags are consistent.
92-105: net9.0-windows defaults to C# 12+, so[]collection expressions are supported
All projects target net9.0-windows without an explicit<LangVersion>, and the default C# compiler version (≥ 12) includes collection expressions. No changes required.Plugins/Flow.Launcher.Plugin.BrowserBookmark/ViewModels/CustomBrowserSettingViewModel.cs (1)
88-107: Dispose FolderBrowserDialog to release native handles.Duplicate of earlier feedback; use a using statement.
- var dialog = new System.Windows.Forms.FolderBrowserDialog - { - Description = "Select the browser's profile data directory.", - UseDescriptionForTitle = true - }; + using var dialog = new System.Windows.Forms.FolderBrowserDialog + { + Description = "Select the browser's profile data directory.", + UseDescriptionForTitle = true + };Based on learnings.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (2)
71-72: LGTM: Volatile.Read used for safe publication in Query.This pairs correctly with Volatile.Write in reload paths.
156-170: LGTM: PeriodicTimer loop hardened against disposal races.Local snapshot + catching ObjectDisposedException/OperationCanceledException is correct.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (3)
104-161: LGTM: Host-level caching + atomic temp-to-final writes.Good dedup by authority, safe concurrent writes via temp file + move, and early local DB path before web fetch.
50-53: Security: Ensure HttpClient validates TLS certs.FaviconWebClient previously disabled cert validation. Confirm it now uses default validation and sane timeouts; otherwise revert the override.
165-190: Task.WaitAsync support confirmed: all projects target net9.0-windows, so Task.WaitAsync is available—no changes required.Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (1)
53-54: Bookmark equality confirmed Bookmark is arecordthat overridesEquals(Bookmark?)andGetHashCode()to compareNameandUrl, soDistinct()will correctly dedupe on those fields.
| var newFirefoxBookmarks = successfulResults.SelectMany(r => r.Bookmarks).ToList(); | ||
| var successfulLoaderNames = successfulResults.Select(r => r.Loader.Name).ToHashSet(); | ||
|
|
||
| public void ReloadData() | ||
| { | ||
| ReloadAllBookmarks(); | ||
| var currentBookmarks = Volatile.Read(ref _bookmarks); | ||
|
|
||
| var otherBookmarks = currentBookmarks.Where(b => !successfulLoaderNames.Any(name => b.Source.StartsWith(name, StringComparison.OrdinalIgnoreCase))); | ||
|
|
||
| var newBookmarkList = otherBookmarks.Concat(newFirefoxBookmarks).Distinct().ToList(); | ||
|
|
||
| Volatile.Write(ref _bookmarks, newBookmarkList); | ||
|
|
There was a problem hiding this comment.
Fix: Don’t use StartsWith when removing old Firefox bookmarks.
Using StartsWith can wrongly drop “Firefox (Store)” when only “Firefox” was reloaded. Normalize source to its base name and compare by set membership.
- var newFirefoxBookmarks = successfulResults.SelectMany(r => r.Bookmarks).ToList();
- var successfulLoaderNames = successfulResults.Select(r => r.Loader.Name).ToHashSet();
+ var newFirefoxBookmarks = successfulResults.SelectMany(r => r.Bookmarks).ToList();
+ var successfulLoaderNames = successfulResults.Select(r => r.Loader.Name).ToHashSet(StringComparer.OrdinalIgnoreCase);
@@
- var otherBookmarks = currentBookmarks.Where(b => !successfulLoaderNames.Any(name => b.Source.StartsWith(name, StringComparison.OrdinalIgnoreCase)));
+ var otherBookmarks = currentBookmarks.Where(b => !successfulLoaderNames.Contains(GetBaseSource(b.Source)));
@@
var newBookmarkList = otherBookmarks.Concat(newFirefoxBookmarks).Distinct().ToList();Add helper:
@@
}
+ private static string GetBaseSource(string source)
+ {
+ var idx = source.IndexOf(" (", StringComparison.Ordinal);
+ return idx >= 0 ? source.Substring(0, idx) : source;
+ }
+
private async Task ReloadFirefoxBookmarksAsync()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var newFirefoxBookmarks = successfulResults.SelectMany(r => r.Bookmarks).ToList(); | |
| var successfulLoaderNames = successfulResults.Select(r => r.Loader.Name).ToHashSet(); | |
| public void ReloadData() | |
| { | |
| ReloadAllBookmarks(); | |
| var currentBookmarks = Volatile.Read(ref _bookmarks); | |
| var otherBookmarks = currentBookmarks.Where(b => !successfulLoaderNames.Any(name => b.Source.StartsWith(name, StringComparison.OrdinalIgnoreCase))); | |
| var newBookmarkList = otherBookmarks.Concat(newFirefoxBookmarks).Distinct().ToList(); | |
| Volatile.Write(ref _bookmarks, newBookmarkList); | |
| var newFirefoxBookmarks = successfulResults.SelectMany(r => r.Bookmarks).ToList(); | |
| var successfulLoaderNames = successfulResults | |
| .Select(r => r.Loader.Name) | |
| .ToHashSet(StringComparer.OrdinalIgnoreCase); | |
| var currentBookmarks = Volatile.Read(ref _bookmarks); | |
| var otherBookmarks = currentBookmarks | |
| .Where(b => !successfulLoaderNames.Contains(GetBaseSource(b.Source))); | |
| var newBookmarkList = otherBookmarks | |
| .Concat(newFirefoxBookmarks) | |
| .Distinct() | |
| .ToList(); | |
| Volatile.Write(ref _bookmarks, newBookmarkList); |
| var newFirefoxBookmarks = successfulResults.SelectMany(r => r.Bookmarks).ToList(); | |
| var successfulLoaderNames = successfulResults.Select(r => r.Loader.Name).ToHashSet(); | |
| public void ReloadData() | |
| { | |
| ReloadAllBookmarks(); | |
| var currentBookmarks = Volatile.Read(ref _bookmarks); | |
| var otherBookmarks = currentBookmarks.Where(b => !successfulLoaderNames.Any(name => b.Source.StartsWith(name, StringComparison.OrdinalIgnoreCase))); | |
| var newBookmarkList = otherBookmarks.Concat(newFirefoxBookmarks).Distinct().ToList(); | |
| Volatile.Write(ref _bookmarks, newBookmarkList); | |
| private static string GetBaseSource(string source) | |
| { | |
| var idx = source.IndexOf(" (", StringComparison.Ordinal); | |
| return idx >= 0 | |
| ? source.Substring(0, idx) | |
| : source; | |
| } |
🤖 Prompt for AI Agents
In Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs around lines 219 to 229,
avoid using StartsWith to filter out old Firefox bookmarks because it can
incorrectly remove sources like "Firefox (Store)"; instead normalize each
Loader.Name to its base name, build a HashSet of those base names, and filter
currentBookmarks by checking whether the base name of b.Source (normalized the
same way) is contained in that set; add a small helper method to extract the
base name from a source string (e.g., trim and drop parenthetical suffixes) and
use it for both creating successfulLoaderNames and comparing b.Source so
comparisons are exact set membership rather than prefix matching.
| public BookmarkWatcherService() | ||
| { | ||
| _debounceTimer = new Timer(_ => OnBookmarkFileChanged?.Invoke(), null, Timeout.Infinite, Timeout.Infinite); | ||
| } | ||
|
|
There was a problem hiding this comment.
Guard timer callback; unhandled exceptions can tear down the process.
Wrap the timer callback in try/catch and no-op when disposed. Invoke subscribers outside the lock.
Apply:
- public BookmarkWatcherService()
- {
- _debounceTimer = new Timer(_ => OnBookmarkFileChanged?.Invoke(), null, Timeout.Infinite, Timeout.Infinite);
- }
+ public BookmarkWatcherService()
+ {
+ _debounceTimer = new Timer(DebounceCallback, null, Timeout.Infinite, Timeout.Infinite);
+ }
+
+ private void DebounceCallback(object? _)
+ {
+ Action? handler;
+ lock (_lock)
+ {
+ if (_disposed) return;
+ handler = OnBookmarkFileChanged;
+ }
+ try { handler?.Invoke(); }
+ catch (Exception ex)
+ {
+ // Swallow to prevent process crash; route through Flow's logger if available.
+ System.Diagnostics.Debug.WriteLine(ex);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public BookmarkWatcherService() | |
| { | |
| _debounceTimer = new Timer(_ => OnBookmarkFileChanged?.Invoke(), null, Timeout.Infinite, Timeout.Infinite); | |
| } | |
| public BookmarkWatcherService() | |
| { | |
| _debounceTimer = new Timer(DebounceCallback, null, Timeout.Infinite, Timeout.Infinite); | |
| } | |
| private void DebounceCallback(object? _) | |
| { | |
| Action? handler; | |
| lock (_lock) | |
| { | |
| if (_disposed) return; | |
| handler = OnBookmarkFileChanged; | |
| } | |
| try | |
| { | |
| handler?.Invoke(); | |
| } | |
| catch (Exception ex) | |
| { | |
| // Swallow to prevent process crash; route through Flow's logger if available. | |
| System.Diagnostics.Debug.WriteLine(ex); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkWatcherService.cs
around lines 17-21, the Timer callback is unguarded: wrap the callback body in a
try/catch to swallow/log exceptions so they cannot bubble out and crash the
process, check a disposed/cancelled flag (or Timer state) at the start and
return no-op if the service is disposed, and ensure OnBookmarkFileChanged
subscribers are invoked outside any locks (capture the delegate into a local
variable then invoke it after releasing locks) to avoid deadlocks and exceptions
propagating from subscriber code.
| if (subElement.TryGetProperty("type", out var type)) | ||
| { | ||
| switch (type.GetString()) | ||
| { | ||
| case "folder": | ||
| case "workspace": // Edge Workspace | ||
| EnumerateFolderBookmark(subElement, bookmarks, source, profilePath); | ||
| break; | ||
| case "url": | ||
| if (subElement.TryGetProperty("name", out var name) && | ||
| subElement.TryGetProperty("url", out var url) && | ||
| !string.IsNullOrEmpty(name.GetString()) && | ||
| !string.IsNullOrEmpty(url.GetString())) | ||
| { | ||
| bookmarks.Add(new Bookmark(name.GetString()!, url.GetString()!, source, profilePath)); | ||
| } | ||
| break; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| _logException(nameof(ChromiumBookmarkLoader), "type property not found in bookmark node.", null); | ||
| } |
There was a problem hiding this comment.
Harden JSON parsing: guard GetString() with ValueKind checks to avoid InvalidOperationException on malformed nodes.
type.GetString(), name.GetString(), and url.GetString() can throw when the JSON value isn’t a string (corrupt file, extensions, experiments). This will bubble into the catch block and drop the entire profile’s bookmarks. Guard the ValueKind and log once per offending node.
Apply this diff:
- if (subElement.TryGetProperty("type", out var type))
+ if (subElement.TryGetProperty("type", out var typeElem))
{
- switch (type.GetString())
+ var typeStr = typeElem.ValueKind == JsonValueKind.String ? typeElem.GetString() : null;
+ if (string.IsNullOrEmpty(typeStr))
+ {
+ _logException(nameof(ChromiumBookmarkLoader), "type property is not a string in bookmark node.", null);
+ continue;
+ }
+ switch (typeStr)
{
case "folder":
case "workspace": // Edge Workspace
EnumerateFolderBookmark(subElement, bookmarks, source, profilePath);
break;
case "url":
- if (subElement.TryGetProperty("name", out var name) &&
- subElement.TryGetProperty("url", out var url) &&
- !string.IsNullOrEmpty(name.GetString()) &&
- !string.IsNullOrEmpty(url.GetString()))
+ var hasName = subElement.TryGetProperty("name", out var nameElem);
+ var hasUrl = subElement.TryGetProperty("url", out var urlElem);
+ var nameStr = hasName && nameElem.ValueKind == JsonValueKind.String ? nameElem.GetString() : null;
+ var urlStr = hasUrl && urlElem.ValueKind == JsonValueKind.String ? urlElem.GetString() : null;
+ if (!string.IsNullOrEmpty(nameStr) && !string.IsNullOrEmpty(urlStr))
{
- bookmarks.Add(new Bookmark(name.GetString()!, url.GetString()!, source, profilePath));
+ bookmarks.Add(new Bookmark(nameStr!, urlStr!, source, profilePath));
}
break;
}
}
else
{
_logException(nameof(ChromiumBookmarkLoader), "type property not found in bookmark node.", null);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (subElement.TryGetProperty("type", out var type)) | |
| { | |
| switch (type.GetString()) | |
| { | |
| case "folder": | |
| case "workspace": // Edge Workspace | |
| EnumerateFolderBookmark(subElement, bookmarks, source, profilePath); | |
| break; | |
| case "url": | |
| if (subElement.TryGetProperty("name", out var name) && | |
| subElement.TryGetProperty("url", out var url) && | |
| !string.IsNullOrEmpty(name.GetString()) && | |
| !string.IsNullOrEmpty(url.GetString())) | |
| { | |
| bookmarks.Add(new Bookmark(name.GetString()!, url.GetString()!, source, profilePath)); | |
| } | |
| break; | |
| } | |
| } | |
| else | |
| { | |
| _logException(nameof(ChromiumBookmarkLoader), "type property not found in bookmark node.", null); | |
| } | |
| if (subElement.TryGetProperty("type", out var typeElem)) | |
| { | |
| // Safely extract the 'type' as string, or log & skip if it's not a string | |
| var typeStr = typeElem.ValueKind == JsonValueKind.String ? typeElem.GetString() : null; | |
| if (string.IsNullOrEmpty(typeStr)) | |
| { | |
| _logException( | |
| nameof(ChromiumBookmarkLoader), | |
| "type property is not a string in bookmark node.", | |
| null | |
| ); | |
| continue; | |
| } | |
| switch (typeStr) | |
| { | |
| case "folder": | |
| case "workspace": // Edge Workspace | |
| EnumerateFolderBookmark(subElement, bookmarks, source, profilePath); | |
| break; | |
| case "url": | |
| // Safely extract 'name' and 'url' as strings | |
| var hasName = subElement.TryGetProperty("name", out var nameElem); | |
| var hasUrl = subElement.TryGetProperty("url", out var urlElem); | |
| var nameStr = hasName && nameElem.ValueKind == JsonValueKind.String | |
| ? nameElem.GetString() : null; | |
| var urlStr = hasUrl && urlElem.ValueKind == JsonValueKind.String | |
| ? urlElem.GetString() : null; | |
| if (!string.IsNullOrEmpty(nameStr) && !string.IsNullOrEmpty(urlStr)) | |
| { | |
| bookmarks.Add(new Bookmark( | |
| nameStr!, | |
| urlStr!, | |
| source, | |
| profilePath | |
| )); | |
| } | |
| break; | |
| } | |
| } | |
| else | |
| { | |
| _logException( | |
| nameof(ChromiumBookmarkLoader), | |
| "type property not found in bookmark node.", | |
| null | |
| ); | |
| } |
🤖 Prompt for AI Agents
In
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs
around lines 106 to 128, the code calls GetString() on JSON properties without
checking ValueKind which can throw for non-string nodes; update the switch and
bookmark-add logic to first verify property.ValueKind == JsonValueKind.String
for type, name and url before calling GetString(), skip and call _logException
once for that offending node when any required property is missing or not a
string, and ensure you continue processing other nodes instead of throwing.
| private async Task<byte[]?> GetFaviconFromDbAsync(Bookmark bookmark, string dbFileName, string query, | ||
| Func<byte[], CancellationToken, Task<byte[]>>? postProcessor, CancellationToken token) | ||
| { | ||
| var dbPath = Path.Combine(bookmark.ProfilePath, dbFileName); | ||
| if (!File.Exists(dbPath)) | ||
| return null; | ||
|
|
There was a problem hiding this comment.
Guard against missing profile paths before hitting the DB
When bookmark.ProfilePath is null or empty (e.g., legacy entries or loaders that haven't populated it yet), this call to Path.Combine throws ArgumentNullException, crashing the favicon pass for every bookmark sourced from that browser. We should bail out early when the profile root is unavailable instead of attempting a DB read.
Apply this diff to harden the method:
- var dbPath = Path.Combine(bookmark.ProfilePath, dbFileName);
+ var profilePath = bookmark.ProfilePath;
+ if (string.IsNullOrWhiteSpace(profilePath) || !Directory.Exists(profilePath))
+ return null;
+
+ var dbPath = Path.Combine(profilePath, dbFileName);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async Task<byte[]?> GetFaviconFromDbAsync(Bookmark bookmark, string dbFileName, string query, | |
| Func<byte[], CancellationToken, Task<byte[]>>? postProcessor, CancellationToken token) | |
| { | |
| var dbPath = Path.Combine(bookmark.ProfilePath, dbFileName); | |
| if (!File.Exists(dbPath)) | |
| return null; | |
| private async Task<byte[]?> GetFaviconFromDbAsync(Bookmark bookmark, string dbFileName, string query, | |
| Func<byte[], CancellationToken, Task<byte[]>>? postProcessor, CancellationToken token) | |
| { | |
| var profilePath = bookmark.ProfilePath; | |
| if (string.IsNullOrWhiteSpace(profilePath) || !Directory.Exists(profilePath)) | |
| return null; | |
| var dbPath = Path.Combine(profilePath, dbFileName); | |
| if (!File.Exists(dbPath)) | |
| return null; | |
| // …rest of the method… | |
| } |
🤖 Prompt for AI Agents
In
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs
around lines 63 to 69, Path.Combine is called with bookmark.ProfilePath which
can be null/empty and will throw ArgumentNullException; guard by checking if
string.IsNullOrWhiteSpace(bookmark?.ProfilePath) (or bookmark.ProfilePath ==
null) at method start and return null immediately before calling Path.Combine,
so we bail out early for missing profile roots and avoid attempting a DB read.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| [RelayCommand] | ||
| private void BrowseDataDirectory() | ||
| { | ||
| var dialog = new System.Windows.Forms.FolderBrowserDialog |
There was a problem hiding this comment.
Using Windows Forms dialogs in a WPF application is not recommended. Consider using a WPF-native folder picker or the newer Windows API Code Pack FolderBrowserDialog for better consistency with the application's UI framework.
Copilot uses AI. Check for mistakes.
| AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate | DecompressionMethods.Brotli | ||
| }; | ||
| _httpClient = new HttpClient(handler); | ||
| _httpClient.DefaultRequestHeaders.UserAgent.ParseAdd("Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/125.0.6409.0 Safari/537.36"); |
There was a problem hiding this comment.
[nitpick] The hardcoded User-Agent string may become outdated and could be flagged by some servers. Consider making this configurable or use a more generic, current User-Agent string.
Copilot uses AI. Check for mistakes.
| catch (IOException) | ||
| { | ||
| // Another thread may have created it concurrently. |
There was a problem hiding this comment.
[nitpick] The catch block silently ignores IOException without logging. Consider adding debug logging to help troubleshoot concurrent file operations.
| catch (IOException) | |
| { | |
| // Another thread may have created it concurrently. | |
| catch (IOException ex) | |
| { | |
| // Another thread may have created it concurrently. | |
| System.Diagnostics.Debug.WriteLine($"IOException during File.Move: {ex.Message}\n{ex.StackTrace}"); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (15)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs (2)
26-26: Use case-insensitive distinct for Windows paths.Avoid duplicate paths caused by casing differences.
- return profileDirs.Distinct(); + return profileDirs.Distinct(System.StringComparer.OrdinalIgnoreCase);
16-16: Harden directory enumeration against IO/ACL exceptions.Enumerating subfolders can throw; ignore and proceed.
- var profileDirs = Directory.EnumerateDirectories(basePath, "Profile *").ToList(); + var profileDirs = new List<string>(); + try + { + profileDirs.AddRange(Directory.EnumerateDirectories(basePath, "Profile *")); + } + catch (System.UnauthorizedAccessException) { /* ignore */ } + catch (IOException) { /* ignore */ }Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs (1)
45-76: Stream bookmarks instead of buffering per profile.Avoids a temporary list and reduces peak memory.
- var bookmarks = new List<Bookmark>(); try { await using var stream = File.OpenRead(bookmarkPath); using var jsonDocument = await JsonDocument.ParseAsync(stream, cancellationToken: cancellationToken); if (jsonDocument.RootElement.TryGetProperty("roots", out var rootElement)) { - bookmarks.AddRange(EnumerateBookmarks(rootElement, source, profilePath)); + foreach (var b in EnumerateBookmarks(rootElement, source, profilePath)) + { + yield return b; + } } } catch (IOException ex) { Main.Context.API.LogException(nameof(ChromiumBookmarkLoader), $"IO error reading {_browserName} bookmarks: {bookmarkPath}", ex); } catch (UnauthorizedAccessException ex) { Main.Context.API.LogException(nameof(ChromiumBookmarkLoader), $"Unauthorized to read {_browserName} bookmarks: {bookmarkPath}", ex); } catch (JsonException ex) { Main.Context.API.LogException(nameof(ChromiumBookmarkLoader), $"Failed to parse bookmarks file for {_browserName}: {bookmarkPath}", ex); } catch (Exception ex) { Main.Context.API.LogException(nameof(ChromiumBookmarkLoader), $"Unexpected error loading {_browserName} bookmarks: {bookmarkPath}", ex); } - - foreach (var bookmark in bookmarks) - { - yield return bookmark; - }Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs (5)
54-56: Avoid O(n^2) scans of the growing StringBuilder.Check only the tail for "" after each append.
- if (contentBuilder.ToString().Contains("</head>", StringComparison.OrdinalIgnoreCase)) + var tailLen = Math.Min(contentBuilder.Length, 2048); + var start = contentBuilder.Length - tailLen; + if (tailLen > 0 && contentBuilder.ToString(start, tailLen).IndexOf("</head>", StringComparison.OrdinalIgnoreCase) >= 0) { break; }
73-79: Ignore non-HTTP(S) favicon URIs early.Prevents unsupported-scheme requests.
public async Task<MemoryStream?> DownloadFaviconAsync(Uri faviconUri, CancellationToken token) { try { + if (faviconUri.Scheme != Uri.UriSchemeHttp && faviconUri.Scheme != Uri.UriSchemeHttps) + return null;
86-88: Pre-allocate MemoryStream when Content-Length is known.Reduces reallocations on larger icons.
- await using var contentStream = await response.Content.ReadAsStreamAsync(token); - var memoryStream = new MemoryStream(); + await using var contentStream = await response.Content.ReadAsStreamAsync(token); + var capacity = response.Content.Headers.ContentLength is long len ? (int)Math.Min(len, MaxFaviconBytes) : 0; + var memoryStream = capacity > 0 ? new MemoryStream(capacity) : new MemoryStream();
25-29: Prefer HTTP/2+ where available.Small perf gains and better TLS/HPACK behavior.
_httpClient = new HttpClient(handler); + _httpClient.DefaultRequestVersion = System.Net.HttpVersion.Version20; + _httpClient.DefaultVersionPolicy = HttpVersionPolicy.RequestVersionOrHigher;
13-128: Favicon cache eviction/cleanup (operational).Introduce size/time-based eviction and orphan cleanup (remove cache entries for URLs no longer in bookmarks). Hook into periodic maintenance or on successful reload. I can draft a lightweight job and wire it to Settings.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (2)
63-96: Consider decoding ICO directly to target size to avoid double transcode.You transcode ICO→PNG with WPF, then decode back with Skia to resize and encode again. If staying with Skia, you could decode the largest ICO frame via
IconBitmapDecoderand resize it usingTransformedBitmapbefore PNG encode, or decode directly with Skia when feasible. This reduces an extra decode/encode cycle and allocations. Not blocking.
98-122: Early bounds check before full bitmap decode (perf/robustness).For very large rasters,
SKBitmap.Decode(stream)will load the entire image. Consider usingSKCodecto read bounds first and bail out (or downscale) when dimensions are extreme (e.g., > 2048px), aligning with favicon expectations.Would you like a small helper using
SKCodecto probe dimensions and enforce a max width/height before decoding?Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (1)
34-35: Redundant cancellation wiring on async enumeration.You already pass
cancellationTokentoGetBookmarksAsync. Adding.WithCancellation(cancellationToken)is redundant and can be removed for clarity.- await foreach (var bookmark in loader.GetBookmarksAsync(cancellationToken).WithCancellation(cancellationToken)) + await foreach (var bookmark in loader.GetBookmarksAsync(cancellationToken)) { bookmarks.Add(bookmark); }Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/HtmlFaviconParser.cs (1)
51-54: Be explicit about string comparison for protocol‑relative URLs.Use
StartsWith("//", StringComparison.Ordinal)for clarity and micro‑perf.- if (href.StartsWith("//")) + if (href.StartsWith("//", StringComparison.Ordinal)) { href = effectiveBaseUri.Scheme + ":" + href; }Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (1)
28-29: Make Firefox polling interval configurable.Hardcoding 3 hours may not fit all users; consider a setting or Environment.ProcessorCount‑aware default.
- private static readonly TimeSpan FirefoxPollingInterval = TimeSpan.FromHours(3); + private static readonly TimeSpan FirefoxPollingInterval = TimeSpan.FromHours(3); // TODO: read from Settings (e.g., Settings.FirefoxPollingIntervalHours)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (2)
100-103: Parallelism: scale to hardware or settings.Use CPU‑aware or setting‑driven MaxDegreeOfParallelism.
- var options = new ParallelOptions { MaxDegreeOfParallelism = 8, CancellationToken = cancellationToken }; + var degree = Math.Max(1, Math.Min(Environment.ProcessorCount, 8)); // or from Settings + var options = new ParallelOptions { MaxDegreeOfParallelism = degree, CancellationToken = cancellationToken };
39-46: Plan cache eviction/cleanup to prevent unbounded growth.Consider a periodic task to prune:
- files older than N days without matching bookmarks,
- keep‑under size cap with LRU,
- remove entries for domains in FailedFetches beyond cooldown.
Happy to sketch an implementation if useful.
Also applies to: 201-279
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs(2 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkWatcherService.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/HtmlFaviconParser.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/IBookmarkLoader.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/IBookmarkLoader.cs
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkWatcherService.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-06T05:26:32.163Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3664
File: Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs:326-326
Timestamp: 2025-06-06T05:26:32.163Z
Learning: In the Firefox bookmark loader (FirefoxBookmarkLoader.cs), using hardcoded Windows line endings ("\r\n") for splitting profiles.ini content is acceptable and platform-agnostic line splitting is not required.
Applied to files:
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.csPlugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs
🧬 Code graph analysis (9)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (6)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Settings(5-106)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (5)
Task(108-127)Task(173-238)List(68-90)List(255-284)Main(16-296)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (3)
Task(94-115)FirefoxBookmarkLoader(13-145)FirefoxBookmarkLoader(32-37)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs (2)
IEnumerable(11-27)BrowserType(29-49)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs (3)
IEnumerable(80-95)ChromiumBookmarkLoader(13-129)ChromiumBookmarkLoader(21-26)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxProfileFinder.cs (4)
FirefoxProfileFinder(9-114)GetFirefoxPlacesPath(11-18)GetFirefoxMsixPlacesPath(20-40)GetPlacesPathFromProfileDir(42-113)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (6)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Settings(5-106)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (7)
LocalFaviconExtractor(12-148)LocalFaviconExtractor(16-19)Task(21-36)Task(38-47)Task(49-59)Task(61-103)Task(105-118)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs (5)
FaviconWebClient(13-128)FaviconWebClient(18-29)Task(31-71)Task(73-121)Dispose(123-127)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/HtmlFaviconParser.cs (2)
HtmlFaviconParser(9-97)List(22-73)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (5)
ImageConverter(13-123)Task(17-38)PngData(40-61)PngData(63-96)PngData(98-122)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (6)
Task(108-127)Task(173-238)Main(16-296)List(68-90)List(255-284)Dispose(286-295)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/HtmlFaviconParser.cs (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (3)
List(68-90)List(255-284)Main(16-296)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (1)
ImageConverter(13-123)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs (3)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (4)
IBookmarkLoader(147-159)IEnumerable(54-57)IEnumerable(59-103)IEnumerable(105-145)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/IBookmarkLoader.cs (1)
IAsyncEnumerable(11-11)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs (2)
BrowserDetector(9-50)IEnumerable(11-27)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (4)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (2)
IBookmarkLoader(147-159)Task(24-52)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/IBookmarkLoader.cs (1)
IAsyncEnumerable(11-11)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (5)
List(68-90)List(255-284)Main(16-296)Task(108-127)Task(173-238)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (6)
CleanupTempFiles(120-147)Task(21-36)Task(38-47)Task(49-59)Task(61-103)Task(105-118)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxProfileFinder.cs (2)
FirefoxProfileFinder(9-114)GetPlacesPathFromProfileDir(42-113)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (7)
Task(73-94)Task(96-161)Task(163-188)Task(201-279)Task(295-316)Task(318-331)Dispose(333-341)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ImageConverter.cs (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (6)
Task(73-94)Task(96-161)Task(163-188)Task(201-279)Task(295-316)Task(318-331)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (4)
Task(21-36)Task(38-47)Task(49-59)Task(61-103)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (8)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Models/Settings.cs (1)
Settings(5-106)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (3)
BookmarkLoaderService(13-160)BookmarkLoaderService(18-22)Task(24-52)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (9)
FaviconService(19-342)FaviconService(35-52)Task(73-94)Task(96-161)Task(163-188)Task(201-279)Task(295-316)Task(318-331)Dispose(333-341)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkWatcherService.cs (4)
BookmarkWatcherService(7-94)BookmarkWatcherService(17-20)UpdateWatchers(22-53)Dispose(76-93)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (1)
Task(94-115)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/LocalFaviconExtractor.cs (5)
Task(21-36)Task(38-47)Task(49-59)Task(61-103)Task(105-118)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs (1)
BrowserType(29-49)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs (2)
SettingsControl(8-67)SettingsControl(13-17)
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs
[warning] 134-134:
msix is not a recognized word. (unrecognized-spelling)
[warning] 132-132:
msix is not a recognized word. (unrecognized-spelling)
[warning] 126-126:
msix is not a recognized word. (unrecognized-spelling)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs
[warning] 171-171:
Cooldown is not a recognized word. (unrecognized-spelling)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs
[warning] 64-64:
shm is not a recognized word. (unrecognized-spelling)
[warning] 63-63:
wal is not a recognized word. (unrecognized-spelling)
[warning] 63-63:
wal is not a recognized word. (unrecognized-spelling)
[warning] 62-62:
wal is not a recognized word. (unrecognized-spelling)
[warning] 59-59:
shm is not a recognized word. (unrecognized-spelling)
[warning] 58-58:
wal is not a recognized word. (unrecognized-spelling)
⏰ 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
🔇 Additional comments (11)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs (3)
56-71: Good: IO/security/JSON/unknown exceptions now handled.This addresses prior review feedback on file-read robustness.
104-121: Harden JSON string extraction (avoidInvalidOperationExceptionon non-strings).Guard
type/name/urlbeforeGetString(). This was flagged previously.- if (subElement.TryGetProperty("type", out var type)) + if (subElement.TryGetProperty("type", out var typeElem)) { - switch (type.GetString()) + var typeStr = typeElem.ValueKind == JsonValueKind.String ? typeElem.GetString() : null; + if (string.IsNullOrEmpty(typeStr)) + { + Main.Context.API.LogWarn(nameof(ChromiumBookmarkLoader), "type property is not a string in bookmark node."); + continue; + } + switch (typeStr) { case "folder": case "workspace": // Edge Workspace EnumerateFolderBookmark(subElement, bookmarks, source, profilePath); break; case "url": - if (subElement.TryGetProperty("name", out var name) && - subElement.TryGetProperty("url", out var url) && - !string.IsNullOrEmpty(name.GetString()) && - !string.IsNullOrEmpty(url.GetString())) + var hasName = subElement.TryGetProperty("name", out var nameElem); + var hasUrl = subElement.TryGetProperty("url", out var urlElem); + var nameStr = hasName && nameElem.ValueKind == JsonValueKind.String ? nameElem.GetString() : null; + var urlStr = hasUrl && urlElem.ValueKind == JsonValueKind.String ? urlElem.GetString() : null; + if (!string.IsNullOrEmpty(nameStr) && !string.IsNullOrEmpty(urlStr)) { - bookmarks.Add(new Bookmark(name.GetString()!, url.GetString()!, source, profilePath)); + bookmarks.Add(new Bookmark(nameStr!, urlStr!, source, profilePath)); } break; } } else { - Main.Context.API.LogException(nameof(ChromiumBookmarkLoader), "type property not found in bookmark node.", null); + Main.Context.API.LogWarn(nameof(ChromiumBookmarkLoader), "type property not found in bookmark node."); }
80-128: No additional unsafeGetString()usages found — all occurrences are confined to ChromiumBookmarkLoader.cs and already guarded by TryGetProperty and null/empty checks.Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconWebClient.cs (1)
20-24: Good: TLS cert validation restored.Removing the dangerous callback closes a critical security hole.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BookmarkLoaderService.cs (2)
96-103: Defensive checks for custom Chromium paths look good.Validates name/path and leverages
BrowserDetector.GetChromiumProfileDirectories, which already includes the base path to cover direct-profile inputs.If you want Brave/Vivaldi/Opera supported out‑of‑the‑box (without custom entries), I can add their known default paths in a follow‑up.
51-52: Confirm deduplication criteria inBookmark.Equals
Bookmarkalready overrides equality to compare onlyNameandUrl, ignoringSourceandProfilePath. Verify that deduplication viaDistinct()should indeed be based solely on those two properties.Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/HtmlFaviconParser.cs (1)
11-20: Good call usingGeneratedRegexover runtime‑compiled regex.This addresses startup/perf concerns from previous reviews about multiple compiled regex instances.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs (1)
21-30: Great fixes: corrected SQL, WAL/SHM snapshot, duplicate‑safe fallback, and stable ProfilePath.
- Query now uses IS NOT NULL and proper JOIN/WHERE.
- Fallback copies -wal/-shm to preserve recent writes.
- Clearing partially read results avoids duplicates.
- ProfilePath derives from original _placesPath, not temp copy.
All align with prior feedback. Based on learnings
Also applies to: 57-66, 67-70, 96-99
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (2)
71-72: Concurrency and visibility look solid.Volatile.Read/Write on _bookmarks and the SemaphoreSlim gate prevent torn reads and overlapping reloads; PeriodicTimer loop is hardened.
Also applies to: 108-127, 158-170
219-226: Bug: StartsWith can drop the wrong sources (e.g., “Firefox (Store)”). Use exact set membership on normalized names.Prefix matching may remove unrelated sources. Normalize base names and compare via HashSet.
Apply this diff:
- var newFirefoxBookmarks = successfulResults.SelectMany(r => r.Bookmarks).ToList(); - var successfulLoaderNames = successfulResults.Select(r => r.Loader.Name).ToHashSet(); + var newFirefoxBookmarks = successfulResults.SelectMany(r => r.Bookmarks).ToList(); + var successfulLoaderNames = successfulResults + .Select(r => GetBaseSource(r.Loader.Name)) + .ToHashSet(StringComparer.OrdinalIgnoreCase); - var currentBookmarks = Volatile.Read(ref _bookmarks); - - var otherBookmarks = currentBookmarks.Where(b => !successfulLoaderNames.Any(name => b.Source.StartsWith(name, StringComparison.OrdinalIgnoreCase))); + var currentBookmarks = Volatile.Read(ref _bookmarks); + var otherBookmarks = currentBookmarks.Where(b => !successfulLoaderNames.Contains(GetBaseSource(b.Source)));Add helper (anywhere in the class, e.g., above ReloadFirefoxBookmarksAsync):
+ private static string GetBaseSource(string source) + { + var idx = source.IndexOf(" (", StringComparison.Ordinal); + return idx >= 0 ? source[..idx] : source; + }Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FaviconService.cs (1)
96-161: Favicon pipeline hardening looks excellent.
- Parallel.ForEachAsync with cancellation honored in GetFaviconFromWebAsync.
- Host/page cache unification + atomic temp‑file writes.
- SHA‑256 cache keys.
- Selection logic guards against null PngData.
- Failure cooldown persisted; SaveFailedFetchesAsync handles ODE.
Also applies to: 163-188, 190-199, 201-279, 281-293
| if (!Directory.Exists(basePath)) | ||
| return []; |
There was a problem hiding this comment.
🧩 Analysis chain
Fix C# 12-only collection expression to avoid build breaks on older LangVersion.
return []; requires C# 12. Use Array.Empty<string>() to be safe.
Apply this diff:
- if (!Directory.Exists(basePath))
- return [];
+ if (!Directory.Exists(basePath))
+ return System.Array.Empty<string>();Run to verify LangVersion and other usages:
🏁 Script executed:
#!/bin/bash
# Check declared C# language versions
fd -t f -a -g "*.csproj" | xargs -I{} sh -c 'echo "== {} =="; rg -n "<LangVersion>" "{}" || echo "(no explicit LangVersion)"'
# Search for other C# 12 collection expressions that may break older compilers
rg -nP --type=cs -C2 $'return\\s*\\[\\s*\\];|=\\s*\\[\\s*\\];'Length of output: 6877
Replace C# 12 collection expressions to avoid build breaks
- No
<LangVersion>is set in any.csproj; the[]collection expression requires C# 12. - Multiple occurrences found (e.g. BrowserDetector.cs, BookmarkWatcherService.cs, Main.cs, Settings.cs, Internationalization.cs, Calculator/Main.cs, Explorer/Settings.cs).
- Replace
return [];→System.Array.Empty<T>()(orEnumerable.Empty<T>())= [];→new List<T>()ornew ObservableCollection<T>()
🤖 Prompt for AI Agents
In Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/BrowserDetector.cs
around lines 13 to 14, the code uses the C# 12 collection expression `return
[];` which breaks builds because no <LangVersion> is set; replace this with a
non-C#12 equivalent such as System.Array.Empty<T>() or Enumerable.Empty<T>()
(use the appropriate T for the method's return type). Also scan other reported
files (BookmarkWatcherService.cs, Main.cs, Settings.cs, Internationalization.cs,
Calculator/Main.cs, Explorer/Settings.cs) and replace `= [];` with a concrete
constructor like new List<T>() or new ObservableCollection<T>() as appropriate
and change any other `return [];` to the array/enumerable empty helper.
| if (!folderElement.TryGetProperty("children", out var childrenElement)) | ||
| return; | ||
|
|
||
| foreach (var subElement in childrenElement.EnumerateArray()) | ||
| { |
There was a problem hiding this comment.
Guard children ValueKind before EnumerateArray() to avoid InvalidOperationException.
Corrupt/extension-modified bookmark trees can set non-array values.
- foreach (var subElement in childrenElement.EnumerateArray())
+ if (childrenElement.ValueKind != JsonValueKind.Array)
+ {
+ Main.Context.API.LogWarn(nameof(ChromiumBookmarkLoader), "children property is not an array in bookmark node.");
+ return;
+ }
+ foreach (var subElement in childrenElement.EnumerateArray())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!folderElement.TryGetProperty("children", out var childrenElement)) | |
| return; | |
| foreach (var subElement in childrenElement.EnumerateArray()) | |
| { | |
| if (!folderElement.TryGetProperty("children", out var childrenElement)) | |
| return; | |
| if (childrenElement.ValueKind != JsonValueKind.Array) | |
| { | |
| Main.Context.API.LogWarn(nameof(ChromiumBookmarkLoader), "children property is not an array in bookmark node."); | |
| return; | |
| } | |
| foreach (var subElement in childrenElement.EnumerateArray()) | |
| { |
🤖 Prompt for AI Agents
In
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/ChromiumBookmarkLoader.cs
around lines 99 to 103, the code calls childrenElement.EnumerateArray() without
verifying that childrenElement is actually a JSON array; add a guard that checks
childrenElement.ValueKind == JsonValueKind.Array (or not JsonValueKind.Null)
before calling EnumerateArray and return or skip processing when it is not an
array to avoid InvalidOperationException from corrupt or extension-modified
bookmark trees.
| catch (Exception e) | ||
| { | ||
| Main.Context.API.LogException(nameof(FirefoxBookmarkLoader), $"Failed to load {_browserName} bookmarks: {_placesPath}", e); | ||
| } |
There was a problem hiding this comment.
Don’t log cancellation as an error; handle OperationCanceledException explicitly.
Currently the generic catch will swallow OCE and log an error. Treat it as a normal shutdown path.
Apply this diff:
- catch (Exception e)
+ catch (OperationCanceledException)
+ {
+ yield break;
+ }
+ catch (Exception e)
{
Main.Context.API.LogException(nameof(FirefoxBookmarkLoader), $"Failed to load {_browserName} bookmarks: {_placesPath}", e);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| catch (Exception e) | |
| { | |
| Main.Context.API.LogException(nameof(FirefoxBookmarkLoader), $"Failed to load {_browserName} bookmarks: {_placesPath}", e); | |
| } | |
| catch (OperationCanceledException) | |
| { | |
| yield break; | |
| } | |
| catch (Exception e) | |
| { | |
| Main.Context.API.LogException( | |
| nameof(FirefoxBookmarkLoader), | |
| $"Failed to load {_browserName} bookmarks: {_placesPath}", | |
| e); | |
| } |
🤖 Prompt for AI Agents
In
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/FirefoxBookmarkLoader.cs
around lines 76 to 79, the generic catch is currently logging
OperationCanceledException as an error; add a specific catch for
OperationCanceledException before the general catch and do not call
Main.Context.API.LogException for it—either swallow or preferably rethrow
(throw;) to preserve the cancellation signal so shutdown is handled normally,
then keep the existing generic Exception catch to log genuine errors.
| Main.Context.API.LogDebug(nameof(Parse), $"Found potential favicon link. Raw tag: '{linkTag}', Extracted href: '{href}', Base URI: '{effectiveBaseUri}'"); | ||
|
|
||
| if (href.StartsWith("//")) | ||
| { | ||
| href = effectiveBaseUri.Scheme + ":" + href; | ||
| } | ||
|
|
||
| if (!Uri.TryCreate(effectiveBaseUri, href, out var fullUrl)) | ||
| { | ||
| Main.Context.API.LogWarn(nameof(Parse), $"Failed to create a valid URI from href: '{href}' and base URI: '{effectiveBaseUri}'"); | ||
| continue; |
There was a problem hiding this comment.
Use class name in logs instead of method name for consistency.
LogDebug/LogWarn are called with nameof(Parse), producing “Parse” as the source. Prefer nameof(HtmlFaviconParser) to align with other logs.
- Main.Context.API.LogDebug(nameof(Parse), $"Found potential favicon link. Raw tag: '{linkTag}', Extracted href: '{href}', Base URI: '{effectiveBaseUri}'");
+ Main.Context.API.LogDebug(nameof(HtmlFaviconParser), $"Found potential favicon link. Raw tag: '{linkTag}', Extracted href: '{href}', Base URI: '{effectiveBaseUri}'");
...
- Main.Context.API.LogWarn(nameof(Parse), $"Failed to create a valid URI from href: '{href}' and base URI: '{effectiveBaseUri}'");
+ Main.Context.API.LogWarn(nameof(HtmlFaviconParser), $"Failed to create a valid URI from href: '{href}' and base URI: '{effectiveBaseUri}'");
...
- Main.Context.API.LogDebug(nameof(Parse), $"Found suitable favicon candidate (score: {score}). Halting further HTML parsing.");
+ Main.Context.API.LogDebug(nameof(HtmlFaviconParser), $"Found suitable favicon candidate (score: {score}). Halting further HTML parsing.");Also applies to: 65-68
🤖 Prompt for AI Agents
In Plugins/Flow.Launcher.Plugin.BrowserBookmark/Services/HtmlFaviconParser.cs
around lines 49-59 (and also apply the same change at 65-68), the
LogDebug/LogWarn calls use nameof(Parse) as the log source; update those calls
to use nameof(HtmlFaviconParser) instead so logs show the class name
consistently with other code. Replace the first argument in each logging call
from nameof(Parse) to nameof(HtmlFaviconParser) for all occurrences in these
ranges and keep the existing message text unchanged.
| public const int TargetIconSize = 48; | ||
|
|
||
| public async Task<(byte[]? PngData, int Size)> ToPngAsync(Stream stream, CancellationToken token) | ||
| { | ||
| token.ThrowIfCancellationRequested(); | ||
|
|
||
| await using var ms = new MemoryStream(); | ||
| await stream.CopyToAsync(ms, token); | ||
| ms.Position = 0; |
There was a problem hiding this comment.
Guard against oversized/hostile inputs when copying streams (potential OOM).
ToPngAsync copies the entire input to memory without a cap. Since inputs can come from the web, a large/hostile image could spike memory and stall the UI process.
Add a conservative limit (e.g., 5 MB) and short‑circuit when exceeded. Also avoid CopyToAsync so you can enforce the cap while reading.
public class ImageConverter
{
public const int TargetIconSize = 48;
+ private const int MaxInputBytes = 5 * 1024 * 1024; // ~5 MB safeguard
public async Task<(byte[]? PngData, int Size)> ToPngAsync(Stream stream, CancellationToken token)
{
token.ThrowIfCancellationRequested();
- await using var ms = new MemoryStream();
- await stream.CopyToAsync(ms, token);
+ await using var ms = new MemoryStream(capacity: 64 * 1024);
+ if (stream.CanSeek && stream.Length > MaxInputBytes)
+ return (null, 0);
+ var buffer = new byte[8192];
+ int read;
+ long total = 0;
+ while ((read = await stream.ReadAsync(buffer.AsMemory(0, buffer.Length), token)) > 0)
+ {
+ total += read;
+ if (total > MaxInputBytes)
+ return (null, 0);
+ await ms.WriteAsync(buffer.AsMemory(0, read), token);
+ }
ms.Position = 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public const int TargetIconSize = 48; | |
| public async Task<(byte[]? PngData, int Size)> ToPngAsync(Stream stream, CancellationToken token) | |
| { | |
| token.ThrowIfCancellationRequested(); | |
| await using var ms = new MemoryStream(); | |
| await stream.CopyToAsync(ms, token); | |
| ms.Position = 0; | |
| public const int TargetIconSize = 48; | |
| private const int MaxInputBytes = 5 * 1024 * 1024; // ~5 MB safeguard | |
| public async Task<(byte[]? PngData, int Size)> ToPngAsync(Stream stream, CancellationToken token) | |
| { | |
| token.ThrowIfCancellationRequested(); | |
| await using var ms = new MemoryStream(capacity: 64 * 1024); | |
| if (stream.CanSeek && stream.Length > MaxInputBytes) | |
| return (null, 0); | |
| var buffer = new byte[8192]; | |
| int read; | |
| long total = 0; | |
| while ((read = await stream.ReadAsync(buffer.AsMemory(0, buffer.Length), token)) > 0) | |
| { | |
| total += read; | |
| if (total > MaxInputBytes) | |
| return (null, 0); | |
| await ms.WriteAsync(buffer.AsMemory(0, read), token); | |
| } | |
| ms.Position = 0; |
There was a problem hiding this comment.
Functionality works well here and I think this is good to go.
However, there are still two things we need to address before merging:
- Please resolve all reviews from AI.
- In
FaviconWebClient, I noticed you have used a newHttpClient.csinstance. But I think it is better to use the one inHttp.cswhich allows Flow to use proxy set by users. So could you please use http API functions (HttpGetStringAsync,HttpGetStreamAsync,HttpDownloadAsync) instead. If you think these api functions are not enough, could you please create a new http api function?
@dcog989 no because this plugin is responsible for handling bookmarks. If the indexing mechanism is better and can be used by other plugins like the Program plugin, then consider turning it into API methods available for others to adopt. |
Old / New Plugin Comparison