fix: don't assign NaN ids to XBEL items missing an @id attribute - #2323
Open
Matovidlo wants to merge 1 commit into
Open
fix: don't assign NaN ids to XBEL items missing an @id attribute#2323Matovidlo wants to merge 1 commit into
Matovidlo wants to merge 1 commit 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.