Skip to content

fix(chat): guard against undefined customModes.custom Fix #272223 and #272236 #272263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
lramos15 merged 1 commit into from
Oct 20, 2025
Merged

fix(chat): guard against undefined customModes.custom Fix #272223 and #272236 #272263

lramos15 merged 1 commit into from
Oct 20, 2025

Conversation

@tamuratak
Copy link
Contributor

@tamuratak tamuratak commented Oct 20, 2025 •
edited
Loading

Guard against undefined customModes.custom. Fix #272223 and #272236
Copilot AI review requested due to automatic review settings October 20, 2025 11:52
Copilot AI reviewed Oct 20, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a runtime error by adding a defensive guard against undefined customModes.custom. The change prevents potential crashes when the custom property is not defined.
@tamuratak tamuratak changed the title fix(chat): guard against undefined customModes.custom Fix #272236 and #272236 Oct 20, 2025
@vs-code-engineering vs-code-engineering bot added this to the October 2025 milestone Oct 20, 2025
@lramos15 lramos15 merged commit afecaaa into microsoft:main Oct 20, 2025
17 checks passed
@lramos15
Copy link
Member

lramos15 commented Oct 20, 2025

Thanks for the contribution
@bpasero
Copy link
Member

bpasero commented Oct 20, 2025

@lramos15 do we have a type problem though, why was this not discovered previously?
@lramos15
Copy link
Member

lramos15 commented Oct 20, 2025

I'll defer to @roblourens since this is his code (I just merged this to unblock the broken picker), but looks like this regressed with #272197. Funny enough Copilot code review caught this #272197 (comment)
@roblourens
Copy link
Member

roblourens commented Oct 20, 2025

Thanks @tamuratak! This is apparently an issue with the type definition of groupBy, because I just had to be fancy. I'll fix it.
@roblourens
Copy link
Member

roblourens commented Oct 20, 2025

Funny enough Copilot code review caught this

The thing is that the first part of the comment modes.custom was previously optional is not helpful because previously there was an unnecessary if check for it (the service used to return an optional custom, but not anymore). And I stopped reading after that. Although it did get to pointing out the subgroup thing correctly.
roblourens added a commit that referenced this pull request Oct 21, 2025
@roblourens
Make type signature of groupBy more accurate
b406544 
See #272263, it currently assumes that all keys of K will be defined. This still isn't perfect, because it says that the returned object might contain a key with value `undefined`, but in reality, if a key is defined, it will have a value. So now usages of this with Object.entries and so on are slighly wrong, but that's preferable IMO.
@roblourens roblourens mentioned this pull request Oct 21, 2025
Merged
roblourens added a commit that referenced this pull request Oct 21, 2025
@roblourens
Make type signature of groupBy more accurate (#272395)
b9480e9 
See #272263, it currently assumes that all keys of K will be defined. This still isn't perfect, because it says that the returned object might contain a key with value `undefined`, but in reality, if a key is defined, it will have a value. So now usages of this with Object.entries and so on are slighly wrong, but that's preferable IMO.
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Dec 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

5 participants

@tamuratak @lramos15 @bpasero @roblourens @Tyriar