Guard splice(indexOf()) against -1 to prevent silent removal of wrong element#314635
Open
SebTardif wants to merge 1 commit intomicrosoft:mainfrom
Open
Guard splice(indexOf()) against -1 to prevent silent removal of wrong element#314635SebTardif wants to merge 1 commit intomicrosoft:mainfrom
SebTardif wants to merge 1 commit intomicrosoft:mainfrom
Conversation
… element When Array.indexOf() returns -1 (element not found), splice(-1, 1) silently removes the last element of the array instead of being a no-op. This can cause incorrect behavior such as removing the wrong keyboard layout, activity badge, progress indicator, or settings profile entry. Add index !== -1 guards before splice calls in: - keyboardLayoutService: removeKeyboardLayout could be called with stale ref - activityService: dispose callback could fire twice (double dispose) - progressService: task may not be in stack if promise resolved before delay - settingsTree: setting may have been removed between checked and splice
Contributor
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @rzhao271Matched files:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens several array-removal sites across the workbench against the splice(-1, 1) footgun by guarding indexOf() results, preventing accidental removal of the last element when the target is not present.
Changes:
- Guard removal from the window progress task stack in
ProgressService. - Guard removal from MRU and keymap layout arrays in
BrowserKeyboardMapperFactoryBase. - Guard removal from activity badge arrays in
ActivityServicedisposables. - Guard removal of a setting key from the “apply to all profiles” list in settings UI logic.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/vs/workbench/services/progress/browser/progressService.ts | Adds idx !== -1 guard before removing a window-progress task from the stack. |
| src/vs/workbench/services/keybinding/browser/keyboardLayoutService.ts | Prevents unintended removal when a keymap layout is missing from _mru / _keymapInfos. |
| src/vs/workbench/services/activity/browser/activityService.ts | Makes activity disposal robust when the activity entry is no longer present in the array. |
| src/vs/workbench/contrib/preferences/browser/settingsTree.ts | Avoids splice(-1, 1) when removing a setting key from the apply-to-all-profiles list. |
rzhao271
approved these changes
May 6, 2026
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.
Problem
When
Array.indexOf()returns-1(element not found),splice(-1, 1)silently removes the last element of the array instead of being a no-op. Several locations in the codebase use this unguarded pattern, which can cause incorrect behavior:removeKeyboardLayoutcould be called with a stale reference, removing the wrong layout from both_mruand_keymapInfosshowViewContainerActivity/showActivitycould fire twice (double dispose), removing the wrong activity badgefinallyblock still attempts to splice itthis.checkedguard (set from a config listener) and the freshgetValue()call; another window/process could have removed the setting between the check and the spliceFix
Add
index !== -1guards before eachsplicecall. This is a standard defensive pattern that costs nothing and prevents a real class of silent data corruption.