Skip to content

fix: don't assign NaN ids to XBEL items missing an @id attribute - #2323

Open
Matovidlo wants to merge 1 commit into
floccusaddon:developfrom
Matovidlo:fix/xbel-missing-id-nan
Open

fix: don't assign NaN ids to XBEL items missing an @id attribute#2323
Matovidlo wants to merge 1 commit into
floccusaddon:developfrom
Matovidlo:fix/xbel-missing-id-nan

Conversation

@Matovidlo

Copy link
Copy Markdown

Fixes #2322

Summary

XbelSerializer._parseFolder parsed an item's id with plain parseInt(node[':@']['@_id']). When the @id attribute is missing or non-numeric, parseInt(undefined) is NaN, which later gets serialized back to disk as the literal string id="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 @id got 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 @id doesn't parse to a real number, assign it a fresh, unique negative id instead of NaN. Negative ids can never collide with the real, ever-incrementing positive ids handed out elsewhere (CachingAdapter.highestId starts at 0 and only increments), and — unlike NaN — 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 compiled src/lib/serializers/Xbel.ts in isolation with tsc and wrote a standalone round-trip check against the compiled, unmodified module:

  • Parse an XBEL folder/bookmark with no @id → before the fix: ids are NaN. After the fix: real, distinct negative ids.
  • Re-serialize and re-parse (simulating the next sync) → ids stay identical across the round-trip, which is exactly the property that was missing before (previously NaN serialized/re-parsed was still non-self-equal in downstream comparisons).

Also ran eslint and tsc --noEmit against the change — both clean.

I'm happy to also add a proper mocha-based unit test for XbelSerializer in 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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

XBEL items with no @id attribute get NaN ids, causing duplicated folders and false-positive deletion failsafe

1 participant