fix: don't assign NaN ids to XBEL items missing an @id attribute - #2323
fix: don't assign NaN ids to XBEL items missing an @id attribute#2323Matovidlo wants to merge 2 commits into
Conversation
XbelSerializer._parseFolder used parseInt() directly on the @id attribute. When a bookmark/folder has no id (or a non-numeric one), parseInt(undefined) returns NaN, which then gets serialized back as the literal string id="NaN". Because NaN !== NaN in JS, any strict-equality identity/hash matching against the cache or local tree on the next sync always fails for these items, even though they're the same folder. Reproduced in an isolated WebDAV test setup: a folder with no @id got triplicated across sync cycles, and the resulting spurious delete+create diff tripped the "would delete N% of your local links" failsafe. Fall back to fresh, unique negative ids (guaranteed not to collide with the ever-incrementing positive highestId-derived ids) instead of NaN, so identity stays stable across repeated sync round-trips.
|
Hi @Matovidlo |
…of using negative ids Items missing a resolvable @id no longer get assigned negative ids as a separate id-space. Instead, XbelSerializer.deserialize() takes the adapter's current highestId and hands out the next id in the same positive, ever-incrementing counter already used for genuinely new items (CachingAdapter's ++this.highestId), then exposes the updated value via XbelSerializer.highestId so WebDav/Git/GoogleDrive/Dropbox adopt it after parsing. This also closes a gap where a stale highestId marker in the file could otherwise let a fallback id collide with a real one.
|
Hi @marcelklehr — fair concern, reworked it to avoid the negative-id space. Items missing a resolvable I did manual testing using Firefox + real WebDAV round trip: missing items resolve to stable ids across repeated syncs, no duplication, and normal create/update flows still work correctly. |
Fixes #2322
Summary
XbelSerializer._parseFolderparsed an item's id with plainparseInt(node[':@']['@_id']). When the@idattribute is missing or non-numeric,parseInt(undefined)isNaN, which later gets serialized back to disk as the literal stringid="NaN".NaN !== NaN, so any subsequent sync round can never match that item back to itself by id — it looks "new" every time. Reproduced end-to-end in an isolated WebDAV test setup: a folder with no@idgot triplicated across sync cycles, and the resulting spurious delete+create diff tripped the "would delete N% of your local links" failsafe (40% in the small repro; up to 100% observed in the wild on a bigger tree).Fix
When an item's
@iddoesn't parse to a real number, assign it a fresh, unique negative id instead ofNaN. Negative ids can never collide with the real, ever-incrementing positive ids handed out elsewhere (CachingAdapter.highestIdstarts at 0 and only increments), and — unlikeNaN— they're stable: once assigned and written back, re-parsing that same id on the next sync yields the same value again, so identity matching stays consistent.Testing
The project's own test suite (
npm test) runs via Selenium against a real browser build, which I couldn't execute in my sandbox. Instead I compiledsrc/lib/serializers/Xbel.tsin isolation withtscand wrote a standalone round-trip check against the compiled, unmodified module:@id→ before the fix: ids areNaN. After the fix: real, distinct negative ids.NaNserialized/re-parsed was still non-self-equal in downstream comparisons).Also ran
eslintandtsc --noEmitagainst the change — both clean.I'm happy to also add a proper mocha-based unit test for
XbelSerializerin the format you'd prefer for this repo — let me know if there's a preferred location for standalone serializer tests, since I didn't see an existing one to follow as a pattern.