Fix stash root detection on folder rename

If a folder that was not being tracked by the model is renamed to have
the `stash_root_name`, we need to explicitly check if it's the stash
root.

I missed this case earlier when teaching the bookmarks model how to have
a partially-loaded tree.

src/mock/browser/bookmarks.ts

@@ -32,6 +32,8 @@ class MockBookmarks implements BM.Static {
   private readonly root: Folder;
   private readonly by_id = new Map<string, Node>();
 
+  private readonly new_bm_parent: Folder;
+
   constructor() {
     this.root = {
       id: this._freeID(),
@@ -42,6 +44,41 @@ class MockBookmarks implements BM.Static {
       dateAdded: 0,
     };
     this.by_id.set(this.root.id, this.root);
+
+    // Yes, these names are deliberately obtuse, to prevent application code
+    // from trying to guess at the bookmark structure. :)
+    this.root.children.push({
+      id: this._freeID(),
+      index: 0,
+      parentId: this.root.id,
+      title: "Toolbin",
+      children: [],
+      dateAdded: 0,
+    });
+    this.root.children.push({
+      id: this._freeID(),
+      index: 0,
+      parentId: this.root.id,
+      title: "Menu Can",
+      children: [],
+      dateAdded: 0,
+    });
+    this.root.children.push({
+      id: this._freeID(),
+      index: 0,
+      parentId: this.root.id,
+      title: "Compost Box",
+      children: [],
+      dateAdded: 0,
+    });
+    for (const c of this.root.children) this.by_id.set(c.id, c);
+
+    this.new_bm_parent = this.root.children[
+      Math.floor(Math.random() * 3)
+    ] as Folder;
+
+    if (!this.new_bm_parent) throw new Error(`bm startup: no new_bm_parent`);
+
     fixup_child_ordering(this.root);
   }
 
@@ -93,7 +130,7 @@ class MockBookmarks implements BM.Static {
       throw new Error(`Bookmark type is not supported on Chrome`);
     }
 
-    const parentId = bookmark.parentId ?? this.root.id;
+    const parentId = bookmark.parentId ?? this.new_bm_parent.id;
     const parent = this._getFolder(parentId);
 
     const index = bookmark.index ?? parent.children.length;

src/model/bookmarks.test.ts

@@ -31,23 +31,35 @@ describe("model/bookmarks - creating the stash root", () => {
   });
 
   it("creates a new stash root when requested", async () => {
+    expect(model.root).to.be.undefined;
+    expect(model.stash_root.value).to.be.undefined;
+
     const p = model.ensureStashRoot();
     await events.next(browser.bookmarks.onCreated);
     const stash_root = await p;
+
     expect(stash_root.title).to.equal(STASH_ROOT_NAME);
     expect(stash_root.isLoaded).to.be.true;
     expect(stash_root.position?.parent).not.to.be.undefined;
   });
 
   it("populates model.root and .stash_root after creation", async () => {
+    expect(model.root).to.be.undefined;
+    expect(model.stash_root.value).to.be.undefined;
+
     const p = model.ensureStashRoot();
     await events.next(browser.bookmarks.onCreated);
     const stash_root = await p;
+
     expect(model.root).not.to.be.undefined;
     expect(model.stash_root.value).to.equal(stash_root);
   });
 
   it("reuses an existing stash root", async () => {
+    await model.ensureStashRoot();
+    expect(model.root).not.to.be.undefined;
+    expect(model.stash_root.value).not.to.be.undefined;
+
     const p = model.ensureStashRoot();
     await events.next(browser.bookmarks.onCreated);
     const stash_root = await p;
@@ -77,6 +89,44 @@ describe("model/bookmarks - creating the stash root", () => {
     expect(sr1.id).to.equal(sr2.id);
     expect(sr2.id).to.equal(sr3.id);
   });
+
+  it("detects when a new stash root is created", async () => {
+    expect(model.root).to.be.undefined;
+    expect(model.stash_root.value).to.be.undefined;
+
+    const btn = await browser.bookmarks.create({title: STASH_ROOT_NAME});
+    await events.next(browser.bookmarks.onCreated);
+
+    await shortPoll(() => {
+      if (!model.root) tryAgain();
+      if (!model.stash_root.value) tryAgain();
+    });
+    expect(model.stash_root.value!.id).to.equal(btn.id);
+  });
+
+  it("detects when a folder is renamed to be the stash root", async () => {
+    const btn = await browser.bookmarks.create({
+      title: `not ${STASH_ROOT_NAME}`,
+    });
+    await events.next(browser.bookmarks.onCreated);
+
+    // Re-create the model - we don't want it seeing the creation event
+    model = await M.Model.from_browser(STASH_ROOT_NAME);
+    expect(model.root).to.be.undefined;
+    expect(model.stash_root.value).to.be.undefined;
+
+    // Created bookmark should not be loaded since we're not tracking its parent
+    expect(model.bookmark(btn.id)).to.be.undefined;
+
+    await browser.bookmarks.update(btn.id, {title: STASH_ROOT_NAME});
+    await events.next(browser.bookmarks.onChanged);
+
+    await shortPoll(() => {
+      if (!model.root) tryAgain();
+      if (!model.stash_root.value) tryAgain();
+    });
+    expect(model.stash_root.value!.id).to.equal(btn.id);
+  });
 });
 
 describe("model/bookmarks", () => {

src/model/bookmarks.ts

@@ -546,6 +546,8 @@ export class Model {
   async ensureStashRoot(): Promise<Folder> {
     if (this.stash_root.value) return this.stash_root.value;
 
+    trace("creating new stash root");
+
     // NOTE: We don't use this.create() here because it's possible the
     // newly-created stash root will never be loaded into the model, if its
     // parent isn't present.  The subsequent call(s) to _findRoots() should take
@@ -565,6 +567,12 @@ export class Model {
     let candidates = await this._findRoots();
     while (Date.now() - start < delay) {
       if (candidates.length > 1) {
+        trace(
+          "race detected; winner = ",
+          candidates[0].id,
+          "loser = ",
+          candidates[1].id,
+        );
         // If we find MULTIPLE candidates so soon after finding NONE,
         // there must be multiple threads trying to create the root
         // folder.  Let's try to remove one.  We are guaranteed that all
@@ -579,6 +587,7 @@ export class Model {
     }
     // END GROSS HACK
 
+    trace("converged on stash root = ", candidates[0].id);
     return candidates[0];
   }
 
@@ -640,7 +649,14 @@ export class Model {
   whenBookmarkChanged(id: string, info: Bookmarks.OnChangedChangeInfoType) {
     trace("whenBookmarkChanged", id, info);
     const node = this.node(id as NodeID);
-    if (!node) return;
+
+    if (!node) {
+      // If we see a node is being renamed to a name that matches the stash
+      // root, we should check if it's becoming the new stash root.
+      if (info.title === this.stash_root_name) this._maybeUpdateStashRoot();
+      return;
+    }
+
     this._updateNode(node, info);
   }
 
@@ -718,13 +734,16 @@ export class Model {
       await browser.bookmarks.search(this.stash_root_name)
     ).filter(c => isBrowserBTNFolder(c) && c.title === this.stash_root_name);
 
+    trace("_findRoots", searched.length, "folders named", this.stash_root_name);
+
     // Make sure those folders and their parents (recursively) are loaded into
     // the model. This is so we can keep an eye on changes and re-trigger the
     // stash-root search if anything changes.
     let to_fetch = new Set(searched.map(c => c.parentId!));
     let to_upsert = Array.from(searched);
 
     while (to_fetch.size > 0) {
+      trace("_findRoots fetching", to_fetch);
       const bms = await browser.bookmarks.get(Array.from(to_fetch));
 
       to_fetch = new Set();
@@ -773,21 +792,17 @@ export class Model {
     if (candidates.length > 0) {
       if (this.stash_root.value !== candidates[0]) {
         this.stash_root.value = candidates[0];
-        trace(
-          "_findStashRootCandidates",
-          "set stash_root to",
-          candidates[0].id,
-        );
+        trace("_findRoots", "set stash_root to", candidates[0].id);
       }
     } else if (this.stash_root.value !== undefined) {
-      trace("_findStashRootCandidates", "cleared stash_root");
+      trace("_findRoots", "cleared stash_root");
       this.stash_root.value = undefined;
     }
 
     // But if we have multiple candidates, we need to raise the alarm that
     // there is an ambiguity the user should resolve.
     if (candidates.length > 1) {
-      trace("_findStashRootCandidates", "found multiple candidates");
+      trace("_findRoots", "found multiple stash_root candidates");
       this.stash_root_warning.value = {
         text:
           `You have multiple "${this.stash_root_name}" bookmark ` +
@@ -797,7 +812,7 @@ export class Model {
         help: () => browser.tabs.create({active: true, url: ROOT_FOLDER_HELP}),
       };
     } else if (this.stash_root_warning.value !== undefined) {
-      trace("_findStashRootCandidates", "found single candidate");
+      trace("_findRoots", "found single stash_root candidate");
       this.stash_root_warning.value = undefined;
     }