Support Loading Msix FireFox Bookmarks & Fix IsRelative logic #3664
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🥷 Code experts: onesounds, Yusyuriv Jack251970, onesounds have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
This comment has been minimized.
This comment has been minimized.
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FirefoxBookmarkLoader
participant FileSystem
User->>FirefoxBookmarkLoader: GetBookmarks()
FirefoxBookmarkLoader->>FileSystem: Locate standard PlacesPath
FirefoxBookmarkLoader->>FileSystem: Locate MsixPlacesPath
FirefoxBookmarkLoader->>FirefoxBookmarkLoader: GetBookmarksFromPath(PlacesPath)
FirefoxBookmarkLoader->>FirefoxBookmarkLoader: GetBookmarksFromPath(MsixPlacesPath)
FirefoxBookmarkLoader->>User: Return combined bookmarks
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs (2)
267-269: Consider adding duplicate bookmark filtering.The method now loads bookmarks from two different Firefox installations and concatenates them. If a user has both standard and MSIX Firefox installed with the same bookmarks, duplicates will appear in the results.
Apply this diff to filter out duplicate bookmarks based on URL:
- var bookmarks1 = GetBookmarksFromPath(PlacesPath); - var bookmarks2 = GetBookmarksFromPath(MsixPlacesPath); - return bookmarks1.Concat(bookmarks2).ToList(); + var bookmarks1 = GetBookmarksFromPath(PlacesPath); + var bookmarks2 = GetBookmarksFromPath(MsixPlacesPath); + // Remove duplicates based on URL + return bookmarks1.Concat(bookmarks2) + .GroupBy(b => b.Url) + .Select(g => g.First()) + .ToList();
310-375: Consider using a proper INI parser for robustness.The current implementation uses basic string manipulation to parse the INI file, which is fragile and doesn't handle all edge cases properly (e.g., spaces around
=, comments, section headers).Would you like me to provide an implementation using a proper INI parser or a more robust parsing approach?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
- GitHub Check: Check Spelling
🔇 Additional comments (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs (1)
277-284: LGTM!Good refactoring to extract the profile path resolution logic into a reusable helper method.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs
Outdated
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs
Outdated
Show resolved
Hide resolved
Jack251970
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Jack251970
added
Dev branch only
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Jack251970
changed the title
| IsRelative=1 | ||
| Path=Profiles/7789f565.default-release | ||
| [Profile2] | ||
| Name=newblahprofile |
There was a problem hiding this comment.
Can you help change this to anything but blah please
There was a problem hiding this comment.
Can you help change this to anything but blah please
Sorry, I have no idea for that. Could you make it little clear?
There was a problem hiding this comment.
I have not changed anything about this in original codes
There was a problem hiding this comment.
Yeah I know, but it's just a profile name right, should be able to find and replace with e.g. 'dummyprofile'?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces support for loading Firefox bookmarks from MSIX installations and fixes issues with the IsRelative logic in bookmark profile paths.
- Added support for reading from an additional MSIX bookmarks path via the new MsixPlacesPath property.
- Updated GetBookmarks to aggregate bookmarks from both traditional and MSIX paths.
- Adjusted the IsRelative logic to handle different string formats from profiles.ini.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. If the flagged items are 🤯 false positivesIf items relate to a ...
|
jjw24
removed
enhancement
Support Loading Msix FireFox Bookmarks & Fix IsRelative logic
Since Win11 has introduced Msix package installer in Microsoft Store, Flow should support path to places.sqlite of MSIX installer:
C:\Users\{UserName}\AppData\Local\Packages\Mozilla.Firefox_n80bbvh6b1yt2\LocalCache\Roaming\Mozilla\Firefox.See more: https://support.mozilla.org/en-US/kb/profiles-where-firefox-stores-user-data#w_finding-your-profile-without-opening-firefox.
Fix IsRelative logic in Fix index out of range error for FireFox bookmarks loading #3659
(relativeAttribute == "0" || relativeAttribute == "IsRelative=0")Fix incorrect Directory.Exists check on file path
Use
File.Existinstead ofDirectory.ExistTest