Fix bookmark last visit time on Chrome

CHANGELOG.md

@@ -6,6 +6,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ## [Unreleased]
 
+### Fixed
+
+- Fix a bug on Chrome where a previously visited bookmark might not display in a
+  larger font even if it's been visited recently.
+
 ## [3.0.0] - 2025-10-10
 
 ### Changed

src/treetop/HistoryManager.test.ts

@@ -1,6 +1,7 @@
 /* eslint no-irregular-whitespace: ["error", { "skipComments": true }] */
 import { SvelteMap } from 'svelte/reactivity';
 import { faker } from '@faker-js/faker';
+import maxBy from 'lodash-es/maxBy';
 import { beforeEach, describe, expect, it, vi } from 'vitest';
 
 import { HistoryManager } from '@Treetop/treetop/HistoryManager';
@@ -14,6 +15,8 @@ import {
   createHistoryItem,
   createOtherBookmarksNode,
   createVisitItem,
+  createVisitItems,
+  type CreateVisitItemsOptions,
 } from '../../test/utils/factories';
 
 const getBookmarkNodes = (
@@ -82,45 +85,63 @@ describe('loadHistory', () => {
     historyManager.init(folderNodeMap);
   });
 
-  it('sets last visit time of visited bookmarks', async () => {
-    const getVisits = vi.fn();
-    vi.spyOn(chrome.history, 'getVisits').mockImplementation(getVisits);
-
-    const visitTimeMap = new Map<string, number>();
-
-    const bookmarkNodes = [];
-    bookmarkNodes.push(...getBookmarkNodes(folderNode1));
-    bookmarkNodes.push(...getBookmarkNodes(folderNode2));
-    // Bookmarks alternate between unvisited and visited
-    bookmarkNodes.forEach((node, index) => {
-      const visitItems = [];
-      let visitTime = 0;
-      if (index & 1) {
-        const visitItem = createVisitItem();
-        visitTime = visitItem.visitTime!;
-        visitItems.push(visitItem);
+  it.each(['chrome', 'firefox'] as const)(
+    'sets last visit time of visited bookmarks with getVisits order convention for %s',
+    async (order) => {
+      const getVisits = vi.fn();
+      vi.spyOn(chrome.history, 'getVisits').mockImplementation(getVisits);
+
+      const visitTimeMap = new Map<string, number>();
+
+      const bookmarkNodes = [];
+      bookmarkNodes.push(...getBookmarkNodes(folderNode1));
+      bookmarkNodes.push(...getBookmarkNodes(folderNode2));
+      // Bookmarks alternate between unvisited and visited
+      bookmarkNodes.forEach((node, index) => {
+        let visitItems: chrome.history.VisitItem[] = [];
+        let mostRecentVisitTime = 0;
+        const count = index & 1 ? faker.number.int({ min: 1, max: 10 }) : 0;
+        if (count > 0) {
+          visitItems = createVisitItems({ count, order });
+          mostRecentVisitTime =
+            maxBy(visitItems, (item) => item.visitTime)?.visitTime ?? 0;
+        }
+        visitTimeMap.set(node.id, mostRecentVisitTime);
+        getVisits.mockResolvedValueOnce(visitItems);
+      });
+
+      await historyManager.loadHistory(folderNodeMap);
+
+      expect(getVisits).toHaveBeenCalledTimes(7);
+      expect(getVisits).toHaveBeenNthCalledWith(1, {
+        url: bookmarkNodes[0].url,
+      });
+      expect(getVisits).toHaveBeenNthCalledWith(2, {
+        url: bookmarkNodes[1].url,
+      });
+      expect(getVisits).toHaveBeenNthCalledWith(3, {
+        url: bookmarkNodes[2].url,
+      });
+      expect(getVisits).toHaveBeenNthCalledWith(4, {
+        url: bookmarkNodes[3].url,
+      });
+      expect(getVisits).toHaveBeenNthCalledWith(5, {
+        url: bookmarkNodes[4].url,
+      });
+      expect(getVisits).toHaveBeenNthCalledWith(6, {
+        url: bookmarkNodes[5].url,
+      });
+      expect(getVisits).toHaveBeenNthCalledWith(7, {
+        url: bookmarkNodes[6].url,
+      });
+
+      expect(lastVisitTimeMap.size).toBe(7);
+      for (const [nodeId, lastVisitTime] of lastVisitTimeMap.entries()) {
+        const visitTime = visitTimeMap.get(nodeId) ?? 0;
+        expect(lastVisitTime).toBe(visitTime);
       }
-      visitTimeMap.set(node.id, visitTime);
-      getVisits.mockResolvedValueOnce(visitItems);
-    });
-
-    await historyManager.loadHistory(folderNodeMap);
-
-    expect(getVisits).toHaveBeenCalledTimes(7);
-    expect(getVisits).toHaveBeenNthCalledWith(1, { url: bookmarkNodes[0].url });
-    expect(getVisits).toHaveBeenNthCalledWith(2, { url: bookmarkNodes[1].url });
-    expect(getVisits).toHaveBeenNthCalledWith(3, { url: bookmarkNodes[2].url });
-    expect(getVisits).toHaveBeenNthCalledWith(4, { url: bookmarkNodes[3].url });
-    expect(getVisits).toHaveBeenNthCalledWith(5, { url: bookmarkNodes[4].url });
-    expect(getVisits).toHaveBeenNthCalledWith(6, { url: bookmarkNodes[5].url });
-    expect(getVisits).toHaveBeenNthCalledWith(7, { url: bookmarkNodes[6].url });
-
-    expect(lastVisitTimeMap.size).toBe(7);
-    for (const [nodeId, lastVisitTime] of lastVisitTimeMap.entries()) {
-      const visitTime = visitTimeMap.get(nodeId) ?? 0;
-      expect(lastVisitTime).toBe(visitTime);
-    }
-  });
+    },
+  );
 
   it('no-op when called twice in a row', async () => {
     const getVisits = vi.fn();
@@ -185,23 +206,45 @@ describe('handleBookmarkCreated', () => {
     expect(lastVisitTimeMap.get(bookmarkNode.id)!).toBe(0);
   });
 
-  it('sets last visit time for a new visited bookmark', async () => {
-    const baseNode = createOtherBookmarksNode();
-    const bookmarkNode = createBrowserBookmarkNode(baseNode);
-
-    const visitItem = createVisitItem();
-
-    const getVisits = vi.fn().mockResolvedValue([visitItem]);
-    vi.spyOn(chrome.history, 'getVisits').mockImplementation(getVisits);
-
-    await historyManager.handleBookmarkCreated(bookmarkNode.id, bookmarkNode);
-
-    expect(getVisits).toHaveBeenCalledOnce();
-    expect(getVisits).toHaveBeenCalledWith({ url: bookmarkNode.url });
-
-    expect(lastVisitTimeMap.has(bookmarkNode.id)).toBe(true);
-    expect(lastVisitTimeMap.get(bookmarkNode.id)!).toBe(visitItem.visitTime!);
-  });
+  it.each<{ count: number; order: CreateVisitItemsOptions['order'] }>([
+    { count: 0, order: 'chrome' },
+    { count: 0, order: 'firefox' },
+    { count: 1, order: 'chrome' },
+    { count: 1, order: 'firefox' },
+    { count: 2, order: 'chrome' },
+    { count: 2, order: 'firefox' },
+    { count: 3, order: 'chrome' },
+    { count: 3, order: 'firefox' },
+    { count: 4, order: 'chrome' },
+    { count: 4, order: 'firefox' },
+    { count: 5, order: 'chrome' },
+    { count: 5, order: 'firefox' },
+    { count: 9, order: 'chrome' },
+    { count: 9, order: 'firefox' },
+    { count: 10, order: 'chrome' },
+    { count: 10, order: 'firefox' },
+  ])(
+    'sets last visit time of visited bookmarks with getVisits of length $count and order convention for $order',
+    async ({ count, order }) => {
+      const baseNode = createOtherBookmarksNode();
+      const bookmarkNode = createBrowserBookmarkNode(baseNode);
+
+      const visitItems = createVisitItems({ count, order });
+      const mostRecentVisitTime =
+        maxBy(visitItems, (item) => item.visitTime)?.visitTime ?? 0;
+
+      const getVisits = vi.fn().mockResolvedValueOnce(visitItems);
+      vi.spyOn(chrome.history, 'getVisits').mockImplementation(getVisits);
+
+      await historyManager.handleBookmarkCreated(bookmarkNode.id, bookmarkNode);
+
+      expect(getVisits).toHaveBeenCalledOnce();
+      expect(getVisits).toHaveBeenCalledWith({ url: bookmarkNode.url });
+
+      expect(lastVisitTimeMap.has(bookmarkNode.id)).toBe(true);
+      expect(lastVisitTimeMap.get(bookmarkNode.id)!).toBe(mostRecentVisitTime);
+    },
+  );
 
   it('ignores new folders', async () => {
     const baseNode = createOtherBookmarksNode();
@@ -236,29 +279,51 @@ describe('handleBookmarkRemoved', () => {
 });
 
 describe('handleBookmarkChanged', () => {
-  it('updates last visit time of a bookmark when its URL is visited', async () => {
-    const baseNode = createOtherBookmarksNode();
-    const bookmarkNode = createBrowserBookmarkNode(baseNode);
-    lastVisitTimeMap.set(bookmarkNode.id, 0);
-
-    const newUrl = faker.internet.url();
-    const changeInfo: Treetop.BookmarkChangeInfo = {
-      title: bookmarkNode.title,
-      url: newUrl,
-    };
-
-    const visitItem = createVisitItem();
-
-    const getVisits = vi.fn().mockResolvedValue([visitItem]);
-    vi.spyOn(chrome.history, 'getVisits').mockImplementation(getVisits);
-
-    await historyManager.handleBookmarkChanged(bookmarkNode.id, changeInfo);
-
-    expect(getVisits).toHaveBeenCalledOnce();
-    expect(getVisits).toHaveBeenCalledWith({ url: newUrl });
-
-    expect(lastVisitTimeMap.get(bookmarkNode.id)!).toBe(visitItem.visitTime!);
-  });
+  it.each<{ count: number; order: CreateVisitItemsOptions['order'] }>([
+    { count: 0, order: 'chrome' },
+    { count: 0, order: 'firefox' },
+    { count: 1, order: 'chrome' },
+    { count: 1, order: 'firefox' },
+    { count: 2, order: 'chrome' },
+    { count: 2, order: 'firefox' },
+    { count: 3, order: 'chrome' },
+    { count: 3, order: 'firefox' },
+    { count: 4, order: 'chrome' },
+    { count: 4, order: 'firefox' },
+    { count: 5, order: 'chrome' },
+    { count: 5, order: 'firefox' },
+    { count: 9, order: 'chrome' },
+    { count: 9, order: 'firefox' },
+    { count: 10, order: 'chrome' },
+    { count: 10, order: 'firefox' },
+  ])(
+    'updates last visit time of a bookmark when its URL is visited for getVisits of length $count and order convention for $order',
+    async ({ count, order }) => {
+      const baseNode = createOtherBookmarksNode();
+      const bookmarkNode = createBrowserBookmarkNode(baseNode);
+      lastVisitTimeMap.set(bookmarkNode.id, 0);
+
+      const newUrl = faker.internet.url();
+      const changeInfo: Treetop.BookmarkChangeInfo = {
+        title: bookmarkNode.title,
+        url: newUrl,
+      };
+
+      const visitItems = createVisitItems({ count, order });
+      const mostRecentVisitTime =
+        maxBy(visitItems, (item) => item.visitTime)?.visitTime ?? 0;
+
+      const getVisits = vi.fn().mockResolvedValueOnce(visitItems);
+      vi.spyOn(chrome.history, 'getVisits').mockImplementation(getVisits);
+
+      await historyManager.handleBookmarkChanged(bookmarkNode.id, changeInfo);
+
+      expect(getVisits).toHaveBeenCalledOnce();
+      expect(getVisits).toHaveBeenCalledWith({ url: newUrl });
+
+      expect(lastVisitTimeMap.get(bookmarkNode.id)!).toBe(mostRecentVisitTime);
+    },
+  );
 
   it('resets last visit time of a bookmark when its URL is not visited', async () => {
     const baseNode = createOtherBookmarksNode();

src/treetop/HistoryManager.ts

@@ -1,6 +1,31 @@
 import { isBookmark } from './bookmarktreenode-utils';
 import * as Treetop from './types';
 
+/**
+ * Get the most recently visited item from a list of chrome.history.VisitItems.
+ * The `chrome.history.getVisits` API behaves differently depending on the
+ * browser:
+ *
+ *   Chrome: Visits are sorted in chronological order.
+ *   Firefox: Visits are sorted in reverse chronological order.
+ *
+ * See:
+ * - https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/history/getVisits
+ * - https://bugzilla.mozilla.org/show_bug.cgi?id=1389588
+ *
+ */
+function getMostRecentItem(items: chrome.history.VisitItem[]) {
+  // The most recent item is either the first or the last, depending on the
+  // browser
+  const first = items.at(0);
+  const last = items.at(-1);
+
+  const firstVisitTime = first?.visitTime ?? 0;
+  const lastVisitTime = last?.visitTime ?? 0;
+
+  return firstVisitTime > lastVisitTime ? first : last;
+}
+
 /**
  * Class to initialize and manage updating last visit times for bookmarks.
  */
@@ -53,8 +78,9 @@ export class HistoryManager {
     // Update last visit times
     const results = await Promise.all(promises);
     results.forEach((items, index) => {
-      if (items.length > 0) {
-        this.lastVisitTimeMap.set(nodeIds[index], items[0].visitTime!);
+      const mostRecentItem = getMostRecentItem(items);
+      if (mostRecentItem?.visitTime) {
+        this.lastVisitTimeMap.set(nodeIds[index], mostRecentItem.visitTime);
       }
     });
 
@@ -87,7 +113,8 @@ export class HistoryManager {
       url: bookmark.url!,
     });
 
-    const visitTime = items.length > 0 ? items[0].visitTime! : 0;
+    const mostRecentItem = getMostRecentItem(items);
+    const visitTime = mostRecentItem?.visitTime ?? 0;
     this.lastVisitTimeMap.set(bookmark.id, visitTime);
   }
 
@@ -114,7 +141,8 @@ export class HistoryManager {
       url: changeInfo.url,
     });
 
-    const newLastVisitTime = items.length > 0 ? items[0].visitTime! : 0;
+    const mostRecentItem = getMostRecentItem(items);
+    const newLastVisitTime = mostRecentItem?.visitTime ?? 0;
     this.lastVisitTimeMap.set(id, newLastVisitTime);
   }
 

test/utils/factories.ts

@@ -120,13 +120,53 @@ export const createHistoryItem = (): chrome.history.HistoryItem => {
   };
 };
 
-export const createVisitItem = (): chrome.history.VisitItem => {
+export const createVisitItem = (
+  withProperties?: Partial<chrome.history.VisitItem>,
+): chrome.history.VisitItem => {
   return {
     id: faker.string.uuid(),
     visitId: faker.string.uuid(),
     visitTime: faker.date.past().getTime(),
     referringVisitId: faker.string.uuid(),
     transition: 'link',
     isLocal: true,
+    ...withProperties,
   };
 };
+
+export interface CreateVisitItemsOptions {
+  /** The number of items to create */
+  count: number;
+  /**
+   * The browser convention to follow when ordering the items (Chrome:
+   * chronological order; Firefox: reverse chronological order)
+   */
+  order: 'chrome' | 'firefox';
+}
+
+/**
+ * Create a list of `chrome.history.VisitItem`s, as returned by
+ * `chrome.history.getVisits`, sorted according to the behavior of the specified
+ * browser.
+ */
+export const createVisitItems = (
+  opts: CreateVisitItemsOptions = {
+    count: 1,
+    order: 'firefox',
+  },
+): chrome.history.VisitItem[] => {
+  const { count, order } = opts;
+
+  const to = faker.date.recent();
+  const from = faker.date.recent({ refDate: to });
+
+  const items = faker.date
+    .betweens({ from, to, count })
+    .map((date) => createVisitItem({ visitTime: date.getTime() }));
+
+  if (order === 'firefox') {
+    items.reverse();
+  }
+
+  return items;
+};