test(sync): Making changes to the tree while syncing should not cause trouble

[marcelklehr]

see #1857

[Copilot]

Pull Request Overview

This pull request adds a new test case to ensure that making changes to the bookmark tree while synchronization is in progress does not lead to errors or inconsistent state.

• Added the test "should not be confused by changes while syncing".
• Verifies that the synchronization process correctly processes concurrent modifications.
• Checks consistency of both server and local bookmark trees after sync operations.

Comments suppressed due to low confidence (2)

src/test/test.js:3057

• [nitpick] Consider using a more robust synchronization mechanism instead of a fixed timeout to ensure test stability across various environments.

await new Promise(resolve => setTimeout(resolve, 1500))

src/test/test.js:3046

• [nitpick] The variable names bookmark, bookmark2, and bookmark3 are not very descriptive. Consider renaming them to more meaningful identifiers to enhance code readability.

const bookmark2 = await browser.bookmarks.create({

[Copilot]

Pull Request Overview

This PR addresses issues with tree synchronization by replacing existing clone methods with new copy methods and by refining the behavior of bookmark tree updates during sync. Key changes include:

• Replacing clone/cloneWithLocation with copy/copyWithLocation across multiple modules.
• Refactoring tree scanning and browser interaction logic (e.g. in Default.ts and BrowserAccount.ts) to use new APIs.
• Adding new interfaces and implementations for handling browser tabs and caching.

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/test/test.js	Adds a test to validate that modifying the tree during sync works as expected.
src/lib/strategies/*.ts	Replaces cloneWithLocation with copyWithLocation in sync processes.
src/lib/native/NativeTree.ts	Uses copy() instead of clone() for updating the bookmarks cache.
src/lib/interfaces/Resource.ts	Adds an optional isUsingBrowserTabs method to the resource interfaces.
src/lib/browser/BrowserAccount.ts	Updates root folder retrieval using BrowserTree.getAbsoluteRootFolder().
src/lib/adapters/NextcloudBookmarks.ts	Replaces clone() calls with copy() in various methods.
src/lib/adapters/Caching.ts	Makes bookmarksCache public and updates copy() usage.
src/lib/Tree.ts	Introduces new copy and copyWithLocation methods alongside existing clone methods.
src/lib/Scanner.ts	Replaces clone() calls with copy() to avoid unintended mutations.
src/lib/LocalTabs.ts	Implements isUsingBrowserTabs to support new browser tab behaviors.
src/lib/Diff.ts	Uses copy() in place of clone() within diff generation.
src/lib/CachingTreeWrapper.ts	Wraps resource calls to use the new caching methods.
src/lib/CacheTree.ts	Implements a CacheTree using the modified adapter methods.
src/lib/Account.ts	Adapts account sync process to use the new CachingTreeWrapper.

Comments suppressed due to low confidence (3)

src/lib/Tree.ts:90

• [nitpick] Both clone and copy methods now exist in the Bookmark (and Folder) classes; consider deprecating the old clone methods if copy is intended as the standard to reduce potential confusion.

clone(withHash?: boolean):Bookmark<L> { return Object.create(this) }

src/lib/adapters/Caching.ts:19

• [nitpick] Changing bookmarksCache from protected to public may expose internal state; please confirm that this view-level access is intended and won’t lead to accidental external modifications.

public bookmarksCache: Folder<TItemLocation>

src/test/test.js:3071

• [nitpick] The test expects the server tree to contain bookmark2 while the local tree is later expected to reflect bookmark3. It would help to document the rationale behind this behavior to clarify why changes during sync are not propagated uniformly.

expectTreeEqual( tree, new Folder({ ... children: [ new Bookmark({ title: bookmark.title, url: bookmark.url }), new Bookmark({ title: bookmark2.title, url: bookmark2.url }) ] }), false )