fix(mcp): preserve userDataDir from config file when CLI options don't override#40370
Open
Will-hxw wants to merge 3 commits intomicrosoft:mainfrom
Open
fix(mcp): preserve userDataDir from config file when CLI options don't override#40370Will-hxw wants to merge 3 commits intomicrosoft:mainfrom
Will-hxw wants to merge 3 commits intomicrosoft:mainfrom
Conversation
…sages level param Fixes: microsoft/playwright-mcp#1429 When using zod with zodToJSONSchema, .describe() must come before .default() to ensure the description is properly attached to the schema before the JSON schema is generated.
…t override In mergeConfig(), browserName and isolated are protected with ?? operator to fall back to base config when overrides are undefined. userDataDir was missing this protection, causing CLI options (undefined when --user-data-dir is not passed) to overwrite the config file value with undefined. Fixes: microsoft/playwright-mcp#1446 Fixes: microsoft#340
Author
|
@microsoft-github-policy-service agree |
Member
|
stray change in console, no test for the fix, unclear spread cause (pick should be picking defined only) |
Issue microsoft#1446: mergeConfig() spreads overrides.browser before applying ?? fallback for individual fields. browserName and isolated already had ?? protection but userDataDir was missing, causing config file userDataDir to be overwritten by undefined from CLI parsing. Add test to verify config file userDataDir is preserved when CLI options don't provide a userDataDir value. Fixes microsoft/playwright-mcp#1446
Author
|
Thanks for the review. I've addressed your concerns:
PR now contains only userDataDir ?? protection (1 line) plus test. Please re-review. |
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
2 participants
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.
Summary
mergeConfig(), add??protection foruserDataDirso it falls back to the base config when CLI overrides are undefinedWhy
Issue #1446:
userDataDirin the config file is ignored becausemergeConfig()spreadsoverrides.browser(containinguserDataDir: undefinedfrom CLI parsing) before applying the??fallback for individual fields.The bug:
browserNameandisolatedare already protected with??operators inmergeConfig().userDataDirwas missing this protection, soundefinedfrom CLI options would overwrite the config file value.Fix:
userDataDir: overrides.browser?.userDataDir ?? base.browser?.userDataDirValidation
browserNameandisolatedin the same functionRelated Issues