Skip to content

Commit c31a787

Browse files
msmolensmsmolens
committed
Fix bookmark last visit time on Chrome
The `chrome.history.getVisits` API behaves differently on different browsers: Firefox sorts the visits in reverse chronological order and Chrome sorts the visits in chronological order. Update the HistoryManager to handle both cases. This fixes a bug on Chrome where a previously visited bookmark might not display in a larger font even if it's been visited recently.
1 parent 8a9c865 commit c31a787

File tree

4 files changed

+219
-83
lines changed

4 files changed

+219
-83
lines changed

‎CHANGELOG.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
66

77
## [Unreleased]
88

9+
### Fixed
10+
11+
- Fix a bug on Chrome where a previously visited bookmark might not display in a
12+
larger font even if it's been visited recently.
13+
914
## [3.0.0] - 2025-10-10
1015

1116
### Changed

‎src/treetop/HistoryManager.test.ts‎

Lines changed: 143 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/* eslint no-irregular-whitespace: ["error", { "skipComments": true }] */
22
import { SvelteMap } from 'svelte/reactivity';
33
import { faker } from '@faker-js/faker';
4+
import maxBy from 'lodash-es/maxBy';
45
import { beforeEach, describe, expect, it, vi } from 'vitest';
56

67
import { HistoryManager } from '@Treetop/treetop/HistoryManager';
@@ -14,6 +15,8 @@ import {
1415
createHistoryItem,
1516
createOtherBookmarksNode,
1617
createVisitItem,
18+
createVisitItems,
19+
type CreateVisitItemsOptions,
1720
} from '../../test/utils/factories';
1821

1922
const getBookmarkNodes = (
@@ -82,45 +85,63 @@ describe('loadHistory', () => {
8285
historyManager.init(folderNodeMap);
8386
});
8487

85-
it('sets last visit time of visited bookmarks', async () => {
86-
const getVisits = vi.fn();
87-
vi.spyOn(chrome.history, 'getVisits').mockImplementation(getVisits);
88-
89-
const visitTimeMap = new Map<string, number>();
90-
91-
const bookmarkNodes = [];
92-
bookmarkNodes.push(...getBookmarkNodes(folderNode1));
93-
bookmarkNodes.push(...getBookmarkNodes(folderNode2));
94-
// Bookmarks alternate between unvisited and visited
95-
bookmarkNodes.forEach((node, index) => {
96-
const visitItems = [];
97-
let visitTime = 0;
98-
if (index & 1) {
99-
const visitItem = createVisitItem();
100-
visitTime = visitItem.visitTime!;
101-
visitItems.push(visitItem);
88+
it.each(['chrome', 'firefox'] as const)(
89+
'sets last visit time of visited bookmarks with getVisits order convention for %s',
90+
async (order) => {
91+
const getVisits = vi.fn();
92+
vi.spyOn(chrome.history, 'getVisits').mockImplementation(getVisits);
93+
94+
const visitTimeMap = new Map<string, number>();
95+
96+
const bookmarkNodes = [];
97+
bookmarkNodes.push(...getBookmarkNodes(folderNode1));
98+
bookmarkNodes.push(...getBookmarkNodes(folderNode2));
99+
// Bookmarks alternate between unvisited and visited
100+
bookmarkNodes.forEach((node, index) => {
101+
let visitItems: chrome.history.VisitItem[] = [];
102+
let mostRecentVisitTime = 0;
103+
const count = index & 1 ? faker.number.int({ min: 1, max: 10 }) : 0;
104+
if (count > 0) {
105+
visitItems = createVisitItems({ count, order });
106+
mostRecentVisitTime =
107+
maxBy(visitItems, (item) => item.visitTime)?.visitTime ?? 0;
108+
}
109+
visitTimeMap.set(node.id, mostRecentVisitTime);
110+
getVisits.mockResolvedValueOnce(visitItems);
111+
});
112+
113+
await historyManager.loadHistory(folderNodeMap);
114+
115+
expect(getVisits).toHaveBeenCalledTimes(7);
116+
expect(getVisits).toHaveBeenNthCalledWith(1, {
117+
url: bookmarkNodes[0].url,
118+
});
119+
expect(getVisits).toHaveBeenNthCalledWith(2, {
120+
url: bookmarkNodes[1].url,
121+
});
122+
expect(getVisits).toHaveBeenNthCalledWith(3, {
123+
url: bookmarkNodes[2].url,
124+
});
125+
expect(getVisits).toHaveBeenNthCalledWith(4, {
126+
url: bookmarkNodes[3].url,
127+
});
128+
expect(getVisits).toHaveBeenNthCalledWith(5, {
129+
url: bookmarkNodes[4].url,
130+
});
131+
expect(getVisits).toHaveBeenNthCalledWith(6, {
132+
url: bookmarkNodes[5].url,
133+
});
134+
expect(getVisits).toHaveBeenNthCalledWith(7, {
135+
url: bookmarkNodes[6].url,
136+
});
137+
138+
expect(lastVisitTimeMap.size).toBe(7);
139+
for (const [nodeId, lastVisitTime] of lastVisitTimeMap.entries()) {
140+
const visitTime = visitTimeMap.get(nodeId) ?? 0;
141+
expect(lastVisitTime).toBe(visitTime);
102142
}
103-
visitTimeMap.set(node.id, visitTime);
104-
getVisits.mockResolvedValueOnce(visitItems);
105-
});
106-
107-
await historyManager.loadHistory(folderNodeMap);
108-
109-
expect(getVisits).toHaveBeenCalledTimes(7);
110-
expect(getVisits).toHaveBeenNthCalledWith(1, { url: bookmarkNodes[0].url });
111-
expect(getVisits).toHaveBeenNthCalledWith(2, { url: bookmarkNodes[1].url });
112-
expect(getVisits).toHaveBeenNthCalledWith(3, { url: bookmarkNodes[2].url });
113-
expect(getVisits).toHaveBeenNthCalledWith(4, { url: bookmarkNodes[3].url });
114-
expect(getVisits).toHaveBeenNthCalledWith(5, { url: bookmarkNodes[4].url });
115-
expect(getVisits).toHaveBeenNthCalledWith(6, { url: bookmarkNodes[5].url });
116-
expect(getVisits).toHaveBeenNthCalledWith(7, { url: bookmarkNodes[6].url });
117-
118-
expect(lastVisitTimeMap.size).toBe(7);
119-
for (const [nodeId, lastVisitTime] of lastVisitTimeMap.entries()) {
120-
const visitTime = visitTimeMap.get(nodeId) ?? 0;
121-
expect(lastVisitTime).toBe(visitTime);
122-
}
123-
});
143+
},
144+
);
124145

125146
it('no-op when called twice in a row', async () => {
126147
const getVisits = vi.fn();
@@ -185,23 +206,45 @@ describe('handleBookmarkCreated', () => {
185206
expect(lastVisitTimeMap.get(bookmarkNode.id)!).toBe(0);
186207
});
187208

188-
it('sets last visit time for a new visited bookmark', async () => {
189-
const baseNode = createOtherBookmarksNode();
190-
const bookmarkNode = createBrowserBookmarkNode(baseNode);
191-
192-
const visitItem = createVisitItem();
193-
194-
const getVisits = vi.fn().mockResolvedValue([visitItem]);
195-
vi.spyOn(chrome.history, 'getVisits').mockImplementation(getVisits);
196-
197-
await historyManager.handleBookmarkCreated(bookmarkNode.id, bookmarkNode);
198-
199-
expect(getVisits).toHaveBeenCalledOnce();
200-
expect(getVisits).toHaveBeenCalledWith({ url: bookmarkNode.url });
201-
202-
expect(lastVisitTimeMap.has(bookmarkNode.id)).toBe(true);
203-
expect(lastVisitTimeMap.get(bookmarkNode.id)!).toBe(visitItem.visitTime!);
204-
});
209+
it.each<{ count: number; order: CreateVisitItemsOptions['order'] }>([
210+
{ count: 0, order: 'chrome' },
211+
{ count: 0, order: 'firefox' },
212+
{ count: 1, order: 'chrome' },
213+
{ count: 1, order: 'firefox' },
214+
{ count: 2, order: 'chrome' },
215+
{ count: 2, order: 'firefox' },
216+
{ count: 3, order: 'chrome' },
217+
{ count: 3, order: 'firefox' },
218+
{ count: 4, order: 'chrome' },
219+
{ count: 4, order: 'firefox' },
220+
{ count: 5, order: 'chrome' },
221+
{ count: 5, order: 'firefox' },
222+
{ count: 9, order: 'chrome' },
223+
{ count: 9, order: 'firefox' },
224+
{ count: 10, order: 'chrome' },
225+
{ count: 10, order: 'firefox' },
226+
])(
227+
'sets last visit time of visited bookmarks with getVisits of length $count and order convention for $order',
228+
async ({ count, order }) => {
229+
const baseNode = createOtherBookmarksNode();
230+
const bookmarkNode = createBrowserBookmarkNode(baseNode);
231+
232+
const visitItems = createVisitItems({ count, order });
233+
const mostRecentVisitTime =
234+
maxBy(visitItems, (item) => item.visitTime)?.visitTime ?? 0;
235+
236+
const getVisits = vi.fn().mockResolvedValueOnce(visitItems);
237+
vi.spyOn(chrome.history, 'getVisits').mockImplementation(getVisits);
238+
239+
await historyManager.handleBookmarkCreated(bookmarkNode.id, bookmarkNode);
240+
241+
expect(getVisits).toHaveBeenCalledOnce();
242+
expect(getVisits).toHaveBeenCalledWith({ url: bookmarkNode.url });
243+
244+
expect(lastVisitTimeMap.has(bookmarkNode.id)).toBe(true);
245+
expect(lastVisitTimeMap.get(bookmarkNode.id)!).toBe(mostRecentVisitTime);
246+
},
247+
);
205248

206249
it('ignores new folders', async () => {
207250
const baseNode = createOtherBookmarksNode();
@@ -236,29 +279,51 @@ describe('handleBookmarkRemoved', () => {
236279
});
237280

238281
describe('handleBookmarkChanged', () => {
239-
it('updates last visit time of a bookmark when its URL is visited', async () => {
240-
const baseNode = createOtherBookmarksNode();
241-
const bookmarkNode = createBrowserBookmarkNode(baseNode);
242-
lastVisitTimeMap.set(bookmarkNode.id, 0);
243-
244-
const newUrl = faker.internet.url();
245-
const changeInfo: Treetop.BookmarkChangeInfo = {
246-
title: bookmarkNode.title,
247-
url: newUrl,
248-
};
249-
250-
const visitItem = createVisitItem();
251-
252-
const getVisits = vi.fn().mockResolvedValue([visitItem]);
253-
vi.spyOn(chrome.history, 'getVisits').mockImplementation(getVisits);
254-
255-
await historyManager.handleBookmarkChanged(bookmarkNode.id, changeInfo);
256-
257-
expect(getVisits).toHaveBeenCalledOnce();
258-
expect(getVisits).toHaveBeenCalledWith({ url: newUrl });
259-
260-
expect(lastVisitTimeMap.get(bookmarkNode.id)!).toBe(visitItem.visitTime!);
261-
});
282+
it.each<{ count: number; order: CreateVisitItemsOptions['order'] }>([
283+
{ count: 0, order: 'chrome' },
284+
{ count: 0, order: 'firefox' },
285+
{ count: 1, order: 'chrome' },
286+
{ count: 1, order: 'firefox' },
287+
{ count: 2, order: 'chrome' },
288+
{ count: 2, order: 'firefox' },
289+
{ count: 3, order: 'chrome' },
290+
{ count: 3, order: 'firefox' },
291+
{ count: 4, order: 'chrome' },
292+
{ count: 4, order: 'firefox' },
293+
{ count: 5, order: 'chrome' },
294+
{ count: 5, order: 'firefox' },
295+
{ count: 9, order: 'chrome' },
296+
{ count: 9, order: 'firefox' },
297+
{ count: 10, order: 'chrome' },
298+
{ count: 10, order: 'firefox' },
299+
])(
300+
'updates last visit time of a bookmark when its URL is visited for getVisits of length $count and order convention for $order',
301+
async ({ count, order }) => {
302+
const baseNode = createOtherBookmarksNode();
303+
const bookmarkNode = createBrowserBookmarkNode(baseNode);
304+
lastVisitTimeMap.set(bookmarkNode.id, 0);
305+
306+
const newUrl = faker.internet.url();
307+
const changeInfo: Treetop.BookmarkChangeInfo = {
308+
title: bookmarkNode.title,
309+
url: newUrl,
310+
};
311+
312+
const visitItems = createVisitItems({ count, order });
313+
const mostRecentVisitTime =
314+
maxBy(visitItems, (item) => item.visitTime)?.visitTime ?? 0;
315+
316+
const getVisits = vi.fn().mockResolvedValueOnce(visitItems);
317+
vi.spyOn(chrome.history, 'getVisits').mockImplementation(getVisits);
318+
319+
await historyManager.handleBookmarkChanged(bookmarkNode.id, changeInfo);
320+
321+
expect(getVisits).toHaveBeenCalledOnce();
322+
expect(getVisits).toHaveBeenCalledWith({ url: newUrl });
323+
324+
expect(lastVisitTimeMap.get(bookmarkNode.id)!).toBe(mostRecentVisitTime);
325+
},
326+
);
262327

263328
it('resets last visit time of a bookmark when its URL is not visited', async () => {
264329
const baseNode = createOtherBookmarksNode();

‎src/treetop/HistoryManager.ts‎

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,31 @@
11
import { isBookmark } from './bookmarktreenode-utils';
22
import * as Treetop from './types';
33

4+
/**
5+
* Get the most recently visited item from a list of chrome.history.VisitItems.
6+
* The `chrome.history.getVisits` API behaves differently depending on the
7+
* browser:
8+
*
9+
* Chrome: Visits are sorted in chronological order.
10+
* Firefox: Visits are sorted in reverse chronological order.
11+
*
12+
* See:
13+
* - https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/history/getVisits
14+
* - https://bugzilla.mozilla.org/show_bug.cgi?id=1389588
15+
*
16+
*/
17+
function getMostRecentItem(items: chrome.history.VisitItem[]) {
18+
// The most recent item is either the first or the last, depending on the
19+
// browser
20+
const first = items.at(0);
21+
const last = items.at(-1);
22+
23+
const firstVisitTime = first?.visitTime ?? 0;
24+
const lastVisitTime = last?.visitTime ?? 0;
25+
26+
return firstVisitTime > lastVisitTime ? first : last;
27+
}
28+
429
/**
530
* Class to initialize and manage updating last visit times for bookmarks.
631
*/
@@ -53,8 +78,9 @@ export class HistoryManager {
5378
// Update last visit times
5479
const results = await Promise.all(promises);
5580
results.forEach((items, index) => {
56-
if (items.length > 0) {
57-
this.lastVisitTimeMap.set(nodeIds[index], items[0].visitTime!);
81+
const mostRecentItem = getMostRecentItem(items);
82+
if (mostRecentItem?.visitTime) {
83+
this.lastVisitTimeMap.set(nodeIds[index], mostRecentItem.visitTime);
5884
}
5985
});
6086

@@ -87,7 +113,8 @@ export class HistoryManager {
87113
url: bookmark.url!,
88114
});
89115

90-
const visitTime = items.length > 0 ? items[0].visitTime! : 0;
116+
const mostRecentItem = getMostRecentItem(items);
117+
const visitTime = mostRecentItem?.visitTime ?? 0;
91118
this.lastVisitTimeMap.set(bookmark.id, visitTime);
92119
}
93120

@@ -114,7 +141,8 @@ export class HistoryManager {
114141
url: changeInfo.url,
115142
});
116143

117-
const newLastVisitTime = items.length > 0 ? items[0].visitTime! : 0;
144+
const mostRecentItem = getMostRecentItem(items);
145+
const newLastVisitTime = mostRecentItem?.visitTime ?? 0;
118146
this.lastVisitTimeMap.set(id, newLastVisitTime);
119147
}
120148

‎test/utils/factories.ts‎

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,13 +120,51 @@ export const createHistoryItem = (): chrome.history.HistoryItem => {
120120
};
121121
};
122122

123-
export const createVisitItem = (): chrome.history.VisitItem => {
123+
export const createVisitItem = (
124+
withProperties?: Partial<chrome.history.VisitItem>,
125+
): chrome.history.VisitItem => {
124126
return {
125127
id: faker.string.uuid(),
126128
visitId: faker.string.uuid(),
127129
visitTime: faker.date.past().getTime(),
128130
referringVisitId: faker.string.uuid(),
129131
transition: 'link',
130132
isLocal: true,
133+
...withProperties,
131134
};
132135
};
136+
137+
export interface CreateVisitItemsOptions {
138+
// The number of items to create
139+
count: number;
140+
// The browser convention to follow when ordering the items (Chrome:
141+
// chronological order; Firefox: reverse chronological order)
142+
order: 'chrome' | 'firefox';
143+
}
144+
145+
/**
146+
* Create a list of `chrome.history.VisitItem`s, as returned by
147+
* `chrome.history.getVisits`, sorted according to the behavior of the specified
148+
* browser.
149+
*/
150+
export const createVisitItems = (
151+
opts: CreateVisitItemsOptions = {
152+
count: 1,
153+
order: 'firefox',
154+
},
155+
): chrome.history.VisitItem[] => {
156+
const { count, order } = opts;
157+
158+
const to = faker.date.recent();
159+
const from = faker.date.recent({ refDate: to });
160+
161+
const items = faker.date
162+
.betweens({ from, to, count })
163+
.map((date) => createVisitItem({ visitTime: date.getTime() }));
164+
165+
if (order === 'firefox') {
166+
items.reverse();
167+
}
168+
169+
return items;
170+
};

0 commit comments

Comments
 (0)