fix(widgets): deleting a duplicated widget also deleted the original - #425
Open
shakurt wants to merge 1 commit into
Open
fix(widgets): deleting a duplicated widget also deleted the original#425shakurt wants to merge 1 commit into
shakurt wants to merge 1 commit into
Conversation
…s instance id
Deleting a duplicated widget sometimes removed the original one too.
Three bugs chained together to cause it:
1. resolveLayoutChange('duplicate') ignored the newWidget argument and
generated its own throwaway local id, so the instance id issued by
createUserWidgetApi was discarded and the duplicate entered the layout
with an id the server had never seen.
2. Because that id did not look like a server id, the next sync tried to
remap it. The mapping picked its target with
synced.find(s => s.widgetKey === w.id), which returns the first record
for that widget key. With two instances of the same widget that is the
original's record, so the duplicate was assigned the original's
instance id and the layout ended up holding two widgets with identical
ids.
3. remove filters by instance id, so removing either one dropped both, and
deleteUserWidgetApi was called with the shared id.
Fixes:
- duplicate now uses the supplied newWidget, keeping the server issued
instance id, and refuses an id that is already present in the layout.
- Add buildInstanceIdMap, which prefers the index aligned record, requires
a matching widgetKey, and never hands out an id that another widget
already holds or that an earlier widget claimed in the same pass. Both
sync paths use it.
- Add dedupeInstanceIds to sanitizeLayout so layouts already corrupted by
this bug are repaired on load: the first widget keeps the id and any
later collision is reassigned a fresh one.
- removeWidget only calls deleteUserWidgetApi for real server ids instead
of any non-empty string.
Covered by tests for the id mapping guards, the dedupe repair, and a
duplicate-then-remove case asserting the original survives.
shakurt
changed the title
shakurt
changed the base branch from
feat/free-widget-canvas
to
perf/widget-canvas-drag-performance
August 29, 2026 18:39
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
1 participant
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.
Problem
Duplicating a widget and then deleting the duplicate sometimes removed the original as well.
Root cause
Three bugs chained together.
resolveLayoutChange('duplicate')ignored thenewWidgetargument and generated its own throwaway local id. The instance id issued bycreateUserWidgetApiwas discarded, so the duplicate entered the layout with an id the server had never seen.Because that id did not look like a server id, the next sync tried to remap it. The mapping picked its target with
synced.find(s => s.widgetKey === w.id), which returns the first record for that widget key. With two instances of the same widget that is the original's record, so the duplicate was assigned the original's instance id and the layout ended up holding two widgets with identical ids.removefilters by instance id, so removing either one dropped both, anddeleteUserWidgetApiwas called with the shared id.validateLayoutdoes reject duplicate instance ids, but the id map is applied after the commit inside the sync callback, so it never ran against the corrupted result.Changes
duplicatenow uses the suppliednewWidget, keeping the server issued instance id, and refuses an id already present in the layout.buildInstanceIdMap, which prefers the index aligned record, requires a matchingwidgetKey, and never hands out an id that another widget already holds or that an earlier widget claimed in the same pass. Both sync paths use it.dedupeInstanceIdstosanitizeLayoutso layouts already corrupted by this bug are repaired on load: the first widget keeps the id, any later collision is reassigned a fresh one. Without this, affected users stay broken after upgrading.removeWidgetonly callsdeleteUserWidgetApifor real server ids instead of any non-empty string.Testing
npm test(43 tests),npm run compile,biome checkandnpm run buildall pass. New tests cover the id mapping guards, the dedupe repair, and a duplicate-then-remove case asserting the original survives.Manual check: duplicate a widget, reload so a sync round trip happens, delete the duplicate, confirm the original stays.