feat: default sort setting - #690
Conversation
josh-berry
left a comment
josh-berry
left a comment
There was a problem hiding this comment.
Hi, thanks for sending this over! I've left you some comments below; let me know if you have any questions.
Also, if you wrote any part of this code with AI, please make sure to add an appropriate Co-Authored-By line to your commit and PR description.
| // When stashing multiple tabs, what order should the tabs have inside the | ||
| // destination group? | ||
| multi_tab_stash_sort: { | ||
| default: "date_added", |
There was a problem hiding this comment.
The default should be to preserve the existing behavior—that is, preserve the order of items as provided.
| const tabStashSortCollator = new Intl.Collator(undefined, { | ||
| usage: "sort", | ||
| sensitivity: "base", | ||
| numeric: true, | ||
| }); |
There was a problem hiding this comment.
There is existing sorting functionality / existing collators already defined in bookmarks.ts. Let's leverage those rather than redefining them. (This will also take care of the issue you have sorting URLs—you cannot treat them like strings because they are structured data; they need to be sorted domain-first.)
|
|
||
| /** Returns a copy of the items in the user's preferred order for insertion | ||
| * when stashing multiple tabs. */ | ||
| sortItemsForMultiTabStashInsertion<T extends StashItem>(items: T[]): T[] { |
There was a problem hiding this comment.
sortItemsForInsertion feels like a cleaner name; putting "Tab Stash" in the name seems redundant.
| toFolder: Bookmarks.Folder; | ||
| toIndex?: number; | ||
| allowDuplicates?: boolean; | ||
| insertionOrder?: StashItem[]; |
There was a problem hiding this comment.
This is redundant to items; why do we need to pass the list of items twice?
(Did you generate this PR with AI?)
| await this.bookmarks.move( | ||
| ordered_nodes[i], | ||
| to_folder, | ||
| insertion_start + i, | ||
| ); | ||
| } |
There was a problem hiding this comment.
All of this is redundant; the existing code already inserts items in the order they are provided in items. We should not be touching bookmarks more than necessary (your addition now moves each bookmark twice), because if the user has other extensions installed or sync happens during a move, it increases the risk of us tripping various Firefox edge cases that can cause sync conflicts or get the user's bookmarks DB into an inconsistent state.
All you need to do is update the callers of putItemsInFolder() to pass the items in in the order you want them to be sorted.
This also considerably simplifies the test cases you need to write, because you essentially only need to test your sort function and that the setting you added is applied correctly.
2 participants
This PR adds a setting to specify sort order when inserting multiple tabs. This is useful when stashing windows with a large number of tabs.
Please let me know if you have any concerns / need anything changed for this PR to get merged.