Skip to content

fix(priority-queue): remove() should match items by value, not by priority - #2223

Open
renanmpimentel wants to merge 1 commit into
trekhleb:masterfrom
renanmpimentel:fix/priority-queue-remove-by-value
Open

renanmpimentel wants to merge 1 commit into
trekhleb:masterfrom
renanmpimentel:fix/priority-queue-remove-by-value

Conversation

@renanmpimentel

@renanmpimentel renanmpimentel commented Oct 6, 2026 •

Copy link
Copy Markdown

Problem

PriorityQueue.remove(item) passes customFindingComparator straight to Heap.remove. When it is omitted, Heap.remove falls back to this.compare, which in PriorityQueue compares priorities. So removing one item removes every item that shares its priority:

const pq = new PriorityQueue();
pq.add('a', 1);
pq.add('b', 1);
pq.add('c', 2);
pq.remove('a');
pq.toString();      // 'c'  -> 'b' was removed too
pq.hasValue('b');   // false

Fix

Default the finding comparator of remove() to value comparison (this.compareValue), the same comparator changePriority() already passes explicitly. Callers that pass a custom comparator are unaffected.

Tests

  • New test should remove only the given item and keep items with the same priority: fails on master (hasValue(5) is false), passes with the fix.
  • npm test: 178 suites / 588 tests pass; npm run lint clean.

Found with supertest: mutation testing plus statement-removal analysis, with every finding confirmed by running the regression against the unmodified test.

…ority

PriorityQueue.remove(item) was called without a finding comparator, so
Heap.remove fell back to this.compare, which compares priorities. As a
result, removing one item also removed every other item sharing its
priority:

  pq.add('a', 1); pq.add('b', 1); pq.remove('a'); // 'b' is gone too

Default the finding comparator to value comparison (the same one
changePriority already uses) and add a regression test.
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.

1 participant