Skip to content

fix(mcp): resolve init-page/init-script paths relative to config file#39322

Open
gpxl-dev wants to merge 2 commits intomicrosoft:mainfrom
gpxl-dev:fix/mcp-config-relative-init-paths
Open

fix(mcp): resolve init-page/init-script paths relative to config file#39322
gpxl-dev wants to merge 2 commits intomicrosoft:mainfrom
gpxl-dev:fix/mcp-config-relative-init-paths

Conversation

@gpxl-dev
Copy link

Summary

  • Resolve browser.initPage and browser.initScript paths relative to the config file location when loading MCP/CLI config.
  • Keep support for absolute paths unchanged.
  • Add CLI config tests proving relative initPage and initScript paths work.

Why

I was using Playwright CLI with a coding agent and wanted to keep .playwright/cli.config.json portable. Relative init paths currently fail unless converted to absolute paths, which makes shared configs harder to use across machines.

Testing

  • npx playwright test --config=tests/mcp/playwright.config.ts --project=chrome tests/mcp/cli-config.spec.ts tests/mcp/init-page.spec.ts

@gpxl-dev
Copy link
Author

@microsoft-github-policy-service agree

return configFromCLIOptions(options);
}

export async function loadConfig(configFile: string | undefined): Promise<Config> {
Copy link
Member

Choose a reason for hiding this comment

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

Can I ask for a stylistic change? Rename this to innerLoadConfig and leave it as is, introduce exported loadConfig below that calls innerLoadConfig and fixes paths below. That way you don't need to call it from 3 different places.

Copy link
Author

Choose a reason for hiding this comment

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

pushed a commit that I think covers this

Copy link
Member

@yury-s yury-s left a comment

Choose a reason for hiding this comment

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

A couple nits, otherwise looks good, thanks!


function resolveConfigFileRelativePaths(config: Config, configFile: string): Config {
const configDir = path.dirname(configFile);
const resolveFilePath = (filePath: string) => path.isAbsolute(filePath) ? filePath : path.resolve(configDir, filePath);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const resolveFilePath = (filePath: string) => path.isAbsolute(filePath) ? filePath : path.resolve(configDir, filePath);
const resolveFilePath = (filePath: string) => path.resolve(configDir, filePath);

nit: path.resolve is a noop if filePath is already absolute

if (!configFile)
return {};

async function innerLoadConfig(configFile: string): Promise<Config> {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
async function innerLoadConfig(configFile: string): Promise<Config> {
function innerLoadConfig(configFile: string): Promise<Config> {

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.

3 participants