Properly handle inconsistent indexes in the bookmarks DB

Normalize the indexes we use to insert child bookmarks into their parent
folders, so that we show bookmarks in the same order as Firefox, even if
Firefox returns inconsistent index values (e.g. two bookmarks with the
same index, indexes which are negative, etc.).

Closes #542.

Author: josh-berry

src/model/bookmarks.test.ts

@@ -141,21 +141,23 @@ describe("model/bookmarks", () => {
 
   describe("loads bookmarks from the browser", () => {
     it("creates bookmark objects", () => {
-      // Pick a few at random inside the stash, since bookmarks ones outside the
+      // Pick a few at random inside the stash, since bookmarks outside the
       // stash are only loaded as needed.
-      for (const l of ["one", "nested_child", "helen"]) {
+      for (const l of ["nested_child", "helen", "undyne"]) {
         const template = bms[l as keyof typeof bms];
         const bm = model.node(template.id);
         if (template.url) {
-          expect(bm).to.deep.include({
+          expect(bm, `${l} URL/title to be set`).to.deep.include({
             id: template.id,
             title: template.title,
             url: template.url,
           });
         }
 
         const parent = model.folder(template.parentId!)!;
-        expect(parent).to.have.property("children");
+        expect(parent, `${l} parent.children to exist`).to.have.property(
+          "children",
+        );
         expect(parent.children[template.index!]).to.equal(bm);
       }
     });
@@ -167,8 +169,8 @@ describe("model/bookmarks", () => {
 
         const bm = model.folder(template.id)!;
         expect(bm.id).to.equal(template.id);
-        expect(bm.children.map(bm => bm?.id)).to.deep.equal(
-          template.children.map(c => c.id),
+        expect(bm.children.map(bm => `${bm?.id} ${bm?.title}`)).to.deep.equal(
+          template.children.map(c => `${c.id} ${c.title}`),
         );
       }
     });

src/model/bookmarks.ts

@@ -263,6 +263,7 @@ export class Model {
 
     return this._run_loader(folder, async () => {
       const children = await browser.bookmarks.getChildren(folder.id);
+      fixupIndexes(children);
       for (const c of children) this._upsertNode(c);
 
       folder.isLoaded = true;
@@ -318,6 +319,7 @@ export class Model {
         // we might allow for multiple loaders for the same child to run.
         // Worse, the other loader might be non-recursive, so loadedSubtree()
         // wouldn't actually perform as expected.
+        fixupIndexes(children);
         for (const n of children) {
           const f = this._upsertNode(n) as Folder;
           const c = n.children;
@@ -528,6 +530,8 @@ export class Model {
         if (toIndex > position.index) toIndex--;
       }
     }
+    const oldParent = node.position?.parent;
+    const oldIndex = node.position?.index;
     await browser.bookmarks.move(node.id, {
       parentId: toParent.id,
       index: toIndex,
@@ -536,7 +540,19 @@ export class Model {
       const pos = node.position;
       /* c8 ignore next -- race avoidance */
       if (!pos) tryAgain();
-      if (pos.parent !== toParent || pos.index !== toIndex) tryAgain();
+
+      // We assume the bookmark move has happened even if the bookmark did not
+      // move to where we expect.  This is because the Firefox bookmark DB might
+      // have incorrect indexes in it, so the bookmark might not move to the
+      // position we expect even if no other concurrent moves are happening.
+      // There is unfortunately little we can do about this, so we ignore it.
+      if (pos.parent !== oldParent || pos.index !== oldIndex) return;
+
+      // It's possible the old and new parents/indexes are the same. If so, the
+      // bookmark will have "moved", but the check above will still fail.
+      if (pos.parent === toParent && pos.index === toIndex) return;
+
+      tryAgain();
     });
 
     await this.maybeCleanupEmptyFolder(position.parent);
@@ -1054,3 +1070,11 @@ function isBrowserBTNFolder(bm: Bookmarks.BookmarkTreeNode): boolean {
   if (!("type" in bm) && !("url" in bm)) return true; // for Chrome
   return false;
 }
+
+/** Firefox sometimes returns a list of bookmark children that have negative
+ * indexes, indexes with gaps, or duplicate indexes. We try to normalize these
+ * here so the rest of Tab Stash doesn't complain. See:
+ * https://github.com/josh-berry/tab-stash/issues/542 */
+function fixupIndexes(nodes: Bookmarks.BookmarkTreeNode[]): void {
+  for (let i = 0; i < nodes.length; ++i) nodes[i].index = i;
+}