-
-
Notifications
You must be signed in to change notification settings - Fork 128
Fixes xbrowsersync/app#190 #518
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -77,10 +77,75 @@ export class ChromiumBookmarkService extends WebExtBookmarkService { | |||||||||||||
| return this.$q.all([clearOthers, clearToolbar]).then(() => {}); | ||||||||||||||
| }) | ||||||||||||||
| .catch((err) => { | ||||||||||||||
| // 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(); | ||||||||||||||
| } | ||||||||||||||
| throw new FailedRemoveNativeBookmarksError(undefined, err); | ||||||||||||||
| }); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| private clearNativeBookmarksFallback(): ng.IPromise<void> { | ||||||||||||||
| // Fallback method: directly traverse the bookmark tree to find and clear xBrowserSync containers | ||||||||||||||
| return browser.bookmarks.getTree().then((tree) => { | ||||||||||||||
| const [root] = tree; | ||||||||||||||
| if (!root || !root.children) { | ||||||||||||||
| this.logSvc.logWarning('Unable to access bookmark tree for fallback recovery'); | ||||||||||||||
| return; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| const clearPromises: ng.IPromise<void>[] = []; | ||||||||||||||
|
|
||||||||||||||
| // Search for xBrowserSync containers by title and clear their children | ||||||||||||||
| const findAndClearXbsContainers = (nodes: NativeBookmarks.BookmarkTreeNode[]): void => { | ||||||||||||||
| nodes.forEach((node) => { | ||||||||||||||
| // Check if this node is an xBrowserSync container | ||||||||||||||
| if ( | ||||||||||||||
| node.title === BookmarkContainer.Other || | ||||||||||||||
| node.title === BookmarkContainer.Toolbar || | ||||||||||||||
| node.title === BookmarkContainer.Menu | ||||||||||||||
| ) { | ||||||||||||||
| // Clear all children of this xBrowserSync container | ||||||||||||||
| if (node.children && node.children.length > 0) { | ||||||||||||||
| node.children.forEach((child) => { | ||||||||||||||
| clearPromises.push( | ||||||||||||||
| this.removeNativeBookmarks(child.id).catch((err) => { | ||||||||||||||
| this.logSvc.logWarning( | ||||||||||||||
| `Failed to remove bookmark ${child.id} from ${node.title} during fallback recovery` | ||||||||||||||
| ); | ||||||||||||||
| // Continue with other bookmarks even if one fails | ||||||||||||||
| return this.$q.resolve(); | ||||||||||||||
| }) | ||||||||||||||
| ); | ||||||||||||||
| }); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| // Recursively search children | ||||||||||||||
| if (node.children && node.children.length > 0) { | ||||||||||||||
| findAndClearXbsContainers(node.children); | ||||||||||||||
| } | ||||||||||||||
| }); | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| // Search through all root children and their descendants | ||||||||||||||
| root.children.forEach((child) => { | ||||||||||||||
| if (child.children && child.children.length > 0) { | ||||||||||||||
| findAndClearXbsContainers(child.children); | ||||||||||||||
| } | ||||||||||||||
| }); | ||||||||||||||
|
Comment on lines
+132
to
+136
|
||||||||||||||
| root.children.forEach((child) => { | |
| if (child.children && child.children.length > 0) { | |
| findAndClearXbsContainers(child.children); | |
| } | |
| }); | |
| findAndClearXbsContainers(root.children); |
Copilot
AI
Mar 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
getNativeContainerIds()rejects withContainerNotFoundError, this branch returnsclearNativeBookmarksFallback()directly. If the fallback itself fails (e.g.,browser.bookmarks.getTree()rejects), the caller will now receive a raw error instead of the existingFailedRemoveNativeBookmarksErrorwrapper used by the non-fallback path. Consider wrapping any fallback failure inFailedRemoveNativeBookmarksErroras well to keepclearNativeBookmarks()error semantics consistent.