Skip to content

[native] perf(NativeTree): Don't calculate hash upon loading tree #2195

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
marcelklehr wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

[native] perf(NativeTree): Don't calculate hash upon loading tree #2195

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions src/lib/native/NativeTree.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,32 +3,45 @@ import { Bookmark, Folder, ItemLocation, TItemLocation } from '../Tree'
import Ordering from '../interfaces/Ordering'
import CachingAdapter from '../adapters/Caching'
import IAccountStorage from '../interfaces/AccountStorage'
import { BulkImportResource } from '../interfaces/Resource'
import { BulkImportResource, IHashSettings } from '../interfaces/Resource'

export default class NativeTree extends CachingAdapter implements BulkImportResource<typeof ItemLocation.LOCAL> {
protected location: TItemLocation = ItemLocation.LOCAL

private storage: IAccountStorage
private readonly accountId: string
private saveTimeout: any
private loaded = false

constructor(storage:IAccountStorage) {
super({})
this.storage = storage
this.accountId = this.storage.accountId
this.loaded = false
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loaded is initialized inline (private loaded = false) and then immediately set to false again in the constructor. The constructor assignment is redundant and can be removed to reduce noise.

Suggested change
this.loaded = false

Copilot uses AI. Check for mistakes.
}

async load():Promise<boolean> {
const {value: tree} = await Storage.get({key: `bookmarks[${this.accountId}].tree`})
const {value: highestId} = await Storage.get({key: `bookmarks[${this.accountId}].highestId`})
if (tree) {
const oldHash = this.bookmarksCache && await this.bookmarksCache.cloneWithLocation(false, this.location).hash(this.hashSettings)
// Make sure we use xxhash3 if we have to calculate hash for this
const hashSettings = {
preserveOrder: true,
hashFn: 'xxhash3',
} as IHashSettings
Comment on lines +28 to +31
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

load() now hard-codes preserveOrder: true (and ignores this.hashSettings.preserveOrder). This changes the meaning of the returned changed flag: if the negotiated hash settings have preserveOrder: false, an order-only change on disk would previously be ignored but will now trigger changed === true and an unnecessary sync. Consider deriving preserveOrder from this.hashSettings (fallback to the previous default) and only overriding the hash function to xxhash3 for performance.

Suggested change
const hashSettings = {
preserveOrder: true,
hashFn: 'xxhash3',
} as IHashSettings
const hashSettings: IHashSettings = {
...(this.hashSettings ?? {}),
// Fall back to previous default when not specified, but respect negotiated settings if present
preserveOrder: this.hashSettings?.preserveOrder ?? true,
hashFn: 'xxhash3',
}

Copilot uses AI. Check for mistakes.
Comment on lines +28 to +31
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using a type assertion (} as IHashSettings), prefer a typed constant (const hashSettings: IHashSettings = ...). This preserves compile-time checking (e.g., if THashFunction changes) and avoids masking missing/extra fields.

Suggested change
const hashSettings = {
preserveOrder: true,
hashFn: 'xxhash3',
} as IHashSettings
const hashSettings: IHashSettings = {
preserveOrder: true,
hashFn: 'xxhash3',
}

Copilot uses AI. Check for mistakes.
const oldHash = this.loaded && this.bookmarksCache && await this.bookmarksCache.cloneWithLocation(false, this.location).hash(hashSettings)
this.bookmarksCache = Folder.hydrate(JSON.parse(tree)).copy(false)
const newHash = await this.bookmarksCache.hash(this.hashSettings)
this.highestId = parseInt(highestId)
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseInt(highestId) will produce NaN when the preference key is missing (Capacitor Preferences returns null) and also relies on implicit radix. Using a safe default (e.g., '0') and an explicit base 10 avoids corrupting this.highestId (which would break future ID generation).

Suggested change
this.highestId = parseInt(highestId)
const parsedHighestId = parseInt(highestId ?? '0', 10)
this.highestId = Number.isNaN(parsedHighestId) ? 0 : parsedHighestId

Copilot uses AI. Check for mistakes.
return oldHash && oldHash !== newHash
if (oldHash && this.loaded) {
const newHash = await this.bookmarksCache.hash(hashSettings)
return oldHash !== newHash
} else {
this.loaded = true
return false
}
} else {
await this.save()
this.loaded = true
return false
}
}
Expand Down
Loading