Merge general settings from different configuration sources

[bbiggs]

TLDR

Merge general settings from different configuration sources

Dive Deeper

general settings were missing from the config merging code in settings.js - this adds it in.

Testing Matrix

	🍏	🪟	🐧
npm run	✅	❓	❓
npx	❓	❓	❓
Docker	❓	❓	❓
Podman	❓	-	-
Seatbelt	❓	-	-

Linked issues / bugs

Fixes #7527

[gemini-code-assist]

Summary of Changes

Hello @bbiggs, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a missing piece in the configuration merging logic by ensuring that 'general' settings are correctly combined from different configuration sources. This change enhances the robustness of the application's configuration management, preventing potential issues where general settings might not be applied as expected.

Highlights

• Configuration Merging: Implemented logic in settings.ts to properly merge 'general' settings from system defaults, user, workspace, and system configurations, addressing a previously missing aspect of the configuration merge process.
• Test Coverage: Updated existing test cases in settings.test.ts to validate that 'general' settings are now correctly initialized and merged across various scenarios, ensuring the new merging behavior functions as expected.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request correctly adds merging for general settings from different configuration sources, which was previously missing. The changes in settings.ts follow the existing pattern for merging nested settings, and the tests are updated accordingly.

My review includes two main points:

1. A suggestion to improve the overall settings merge strategy in settings.ts for better maintainability and to prevent future bugs. The current pattern is fragile.
2. A recommendation to add more comprehensive tests for the new general settings merge logic, as it is currently not covered.

[gemini-code-assist]

While this change correctly implements deep merging for general settings, it follows a pattern in this function that is fragile. The top-level object spreads (...systemDefaults, ...user, etc.) perform a shallow merge, which is incorrect for nested objects. This is then compensated for by explicitly deep-merging properties like ui, context, and now general.

This approach is brittle because any new nested setting object will be merged incorrectly if an explicit deep-merge block is not also added.

For improved maintainability and robustness, consider refactoring this function to use a deep-merge utility (e.g., from lodash-es, which is already a dependency) as the primary merge strategy, and then handle special cases like array concatenation separately. This could be addressed in a follow-up PR.

Example of a more robust approach:

import { merge } from 'lodash-es';

// ...
const merged = merge(
  {},
  systemDefaults,
  user,
  safeWorkspaceWithoutFolderTrust,
  system
);
// Then handle array concatenations...

[gemini-code-assist]

This test should be updated to verify that general settings are correctly merged, not just that an empty object is present. The new merging logic for general is currently untested.

Please update this test case by adding general properties to the userSettingsContent and workspaceSettingsContent constants, and then updating this expectation to check for the correctly merged general object.

For example, if you add general: { vimMode: true, preferredEditor: 'emacs' } to user settings and general: { preferredEditor: 'vscode' } to workspace settings, the expected merged general object would be { vimMode: true, preferredEditor: 'vscode' }.

        general: {
          vimMode: true,
          preferredEditor: 'vscode',
        },