Skip to content

Refactor computeVisibleOptionGroups#307284

Merged
mjbvz merged 12 commits intomainfrom
dev/mjbvz/ratty-anteater
Apr 2, 2026
Merged

Refactor computeVisibleOptionGroups#307284
mjbvz merged 12 commits intomainfrom
dev/mjbvz/ratty-anteater

Conversation

@mjbvz
Copy link
Copy Markdown
Collaborator

@mjbvz mjbvz commented Apr 1, 2026

Refactors computeVisibleOptionGroups to be more accurate and hopefully readable. This doesn't get the code into a perfect state but it's better than it was

I have broke this refactoring into a number of commits with comments explaining the change I am making and some of the thought behind each step. One important note is that these are not 100% safe refactorings. This was more of an exercise to show the process how I'd try to understand this single function and clean it up

Some general points:

  • Renaming to give accurate names can be a great way to understand the code and make ugly parts of it obvious

  • It's better to have ugly parts of the code be obvious than hide them away. A name like getOptions may be nice and clean looking but a more accurate name like getOptionsFromServerAndUpdateCacheAndMaybePromptTheUserIfItsAprilFoolsDay is better if that's what the method actually does

  • It's ok to make code more ugly as part of refactoring so long as the ugliness is honest

  • Breaking up code can help you understand how it works, the dependencies between the different parts, and see when one method/function is doing way too much

  • Strive for code blocks that have a single clear responsibility. That responsibility may be very abstract and high level or it may be very low level. Either way, code that does more than one thing is a great candidate for being broken up

  • I also find it's useful to try separating out the stateless parts of the code from the stateful parts. This makes the stateless parts easier to reason about and test, and hopefully lets you reduce and isolate state in the longer term

  • Assume no one will read the docs because largely no one will. Instead try designing apis that are self documenting and suggest the correct use (and more importantly, are difficult to misunderstand/misuse)

  • You can use renaming and extraction as a way to understand code. This is useful to do even if you have no intention of ever checking in the refactoring.

  • Agents can be a great tool for making refactorings, especially ones that traditional refactoring tools would never support. However current agents are not so good at automatically refactoring on their own. They aren't even great when you prompt them to refactor to improve code quality. You have to provide very detailed instructions about what you want

  • If you can't understand the code, coding agents will struggle with it too. If the code isn't understood, it will keep getting worse

mjbvz added 9 commits April 1, 2026 11:56
This function does way more than compute the visible options groups. Let's give it a name that reflects this so it's clear it should be refactored
Start breaking up the function because it's doing too much
Start splitting up methods so that each one has a clearer responsibility
- `optionsGroups` is confusing unless you also know there's `visibleOptionsGroups`

- Then `visibleOptionIds` returns the ids instead of the actual groups? But then in most cases the caller then needs to map back through to get the group so let's just return the group directly?
This duplicates some code but make the actual method cleaner. It also lets us cleanly remove one of the return values to get us closer to being able to have this method return just what it describes
This is not related to what the method says it does
This part also isn't directly related to the original method so should be split out. I'm actually thinking that we may be able to remove it entirely in `refreshChatSessionPickers` since the widgets should only exist for visible options groups, but I don't want to make that change just yet
I'm not a fan of types like this unless there's a clear meaning of `undefined` values. In this case it's just being used to signal that there are no options groups

That makes the code fragile because both the callers and implementers have to agree that the method will not return an empty array. It's likely that either the caller will do extra checks for both `!undefined && !arr.length` or that we will accidentally break this contract in the implementation
Copilot AI review requested due to automatic review settings April 1, 2026 23:21
Copy link
Copy Markdown
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 refactors how ChatInputPart computes visible chat session option groups and updates related context keys, aiming to centralize picker visibility logic and reuse it across session-type changes, rendering, and picker refresh.

Changes:

  • Replaces computeVisibleOptionGroups() with smaller helpers that return visible option groups and update context keys.
  • Updates picker creation/refresh flows to use sessionResource explicitly via a new getCurrentSessionResource() helper.
  • Adjusts effective session type resolution to return undefined instead of '' and updates call sites accordingly.

@mjbvz mjbvz changed the title Dev/mjbvz/ratty anteater Refactor computeVisibleOptionGroups Apr 2, 2026
mjbvz added 3 commits April 1, 2026 23:31
Some of the ugly chat mode stuff was removed after I did this refactoring. The merge I made incorrectly restored that code
@ideedestiny

This comment was marked as spam.

@mjbvz mjbvz marked this pull request as ready for review April 2, 2026 17:04
@mjbvz mjbvz merged commit 4022448 into main Apr 2, 2026
19 checks passed
@mjbvz mjbvz deleted the dev/mjbvz/ratty-anteater branch April 2, 2026 17:11
@vs-code-engineering vs-code-engineering bot added this to the 1.115.0 milestone Apr 2, 2026
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.

4 participants