Conversation
|
I had this problem and this patch fixed it. Thanks! |
There was a problem hiding this comment.
Pull request overview
Improves Chromium bookmark container handling to prevent sync failures when native bookmark root container IDs don’t match the previously hardcoded assumptions (reported in #190).
Changes:
- Adds a position-based fallback in
getNativeContainerIds()when hardcoded root IDs don’t resolve. - Adds a recovery path in
clearNativeBookmarks()to attempt clearing xBrowserSync folders by title when container resolution fails. - Introduces
clearNativeBookmarksFallback()to traverse the bookmark tree and remove items found under[xbs] ...folders.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If container IDs can't be found, try fallback recovery method | ||
| if (err instanceof ContainerNotFoundError) { | ||
| this.logSvc.logWarning('Container IDs not found, attempting fallback recovery to clear bookmarks'); | ||
| return this.clearNativeBookmarksFallback(); |
There was a problem hiding this comment.
When getNativeContainerIds() rejects with ContainerNotFoundError, this branch returns clearNativeBookmarksFallback() directly. If the fallback itself fails (e.g., browser.bookmarks.getTree() rejects), the caller will now receive a raw error instead of the existing FailedRemoveNativeBookmarksError wrapper used by the non-fallback path. Consider wrapping any fallback failure in FailedRemoveNativeBookmarksError as well to keep clearNativeBookmarks() error semantics consistent.
| return this.clearNativeBookmarksFallback(); | |
| return this.clearNativeBookmarksFallback().catch((fallbackErr) => { | |
| throw new FailedRemoveNativeBookmarksError(undefined, fallbackErr); | |
| }); |
| // Fallback: try to find containers by position if hardcoded IDs don't match | ||
| // In Chromium browsers, root typically has exactly 2 children: toolbar (index 0) and other (index 1) | ||
| if ((!otherBookmarksNode || !toolbarBookmarksNode) && root.children && root.children.length === 2) { | ||
| // Try to identify by position - typically toolbar is first, other is second | ||
| if (!toolbarBookmarksNode && root.children[0]) { | ||
| toolbarBookmarksNode = root.children[0]; | ||
| this.logSvc.logInfo(`Found toolbar bookmarks by position: ${toolbarBookmarksNode.id}`); | ||
| } | ||
| if (!otherBookmarksNode && root.children[1]) { | ||
| otherBookmarksNode = root.children[1]; | ||
| this.logSvc.logInfo(`Found other bookmarks by position: ${otherBookmarksNode.id}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
The position-based fallback only runs when root.children.length === 2, but many Chromium bookmark trees have 3+ root children (e.g., including “Mobile bookmarks”). If IDs differ from the hardcoded values in those cases, this fallback will never trigger and ContainerNotFoundError will still be thrown. Consider loosening this condition (e.g., require >= 2 and pick indices 0/1, or add additional heuristics) so the fallback can help on more Chromium variants.
| root.children.forEach((child) => { | ||
| if (child.children && child.children.length > 0) { | ||
| findAndClearXbsContainers(child.children); | ||
| } | ||
| }); |
There was a problem hiding this comment.
clearNativeBookmarksFallback() only calls findAndClearXbsContainers(child.children) for each root.children entry, so it never checks the root.children nodes themselves for [xbs] ... titles (despite the comment saying it searches “root children and their descendants”). Consider running findAndClearXbsContainers(root.children) (or explicitly checking each root child) so the fallback also works if an xBrowserSync container ever exists directly under the root.
| root.children.forEach((child) => { | |
| if (child.children && child.children.length > 0) { | |
| findAndClearXbsContainers(child.children); | |
| } | |
| }); | |
| findAndClearXbsContainers(root.children); |
Problem
The extension fails with "Missing container: other bookmarks" error when the browser's native bookmark container IDs don't match the expected hardcoded values ('1' for toolbar, '2' for other bookmarks). This can occur in certain browser configurations or when the bookmark structure has been modified. See #190
Solution
Added resilient container detection with two fallback mechanisms:
Position-based fallback in
getNativeContainerIds(): When hardcoded IDs don't match, the code now attempts to identify containers by their position in the bookmark tree (toolbar at index 0, other bookmarks at index 1). This works reliably for Chromium browsers which typically have exactly 2 root children.Recovery fallback in
clearNativeBookmarks(): If container IDs still can't be found, the code now traverses the bookmark tree to find xBrowserSync containers by their titles ([xbs] Other,[xbs] Toolbar,[xbs] Menu) and clears them directly. This ensures bookmarks can be reset even when the native container structure is unexpected.Changes
getNativeContainerIds()with position-based container detection fallbackclearNativeBookmarksFallback()method for recovery when container IDs are unavailableclearNativeBookmarks()to catchContainerNotFoundErrorand attempt fallback recoveryTesting
Notes
This is a defensive programming approach that makes the extension resilient to browser implementation variations without attempting to modify the browser's native bookmark structure (which we don't control).