Skip to content

Commit 3f4a7b2

Browse files
skulidropekskulidropek
committed
fix(mcp): address coderabbit review comments
1 parent 54ee160 commit 3f4a7b2

14 files changed

Lines changed: 76 additions & 35 deletions

File tree

‎packages/app/src/lib/core/templates-entrypoint/claude.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ const renderClaudeMcpPlaywrightConfig = (): string =>
193193
String.raw`# Claude Code: keep Playwright MCP config in sync with container settings
194194
CLAUDE_SETTINGS_FILE="${"$"}{CLAUDE_HOME_JSON:-$CLAUDE_CONFIG_DIR/.claude.json}"
195195
docker_git_sync_claude_playwright_mcp() {
196-
CLAUDE_SETTINGS_FILE="$CLAUDE_SETTINGS_FILE" MCP_PLAYWRIGHT_ENABLE="$MCP_PLAYWRIGHT_ENABLE" node - <<'NODE'
196+
CLAUDE_SETTINGS_FILE="$CLAUDE_SETTINGS_FILE" MCP_PLAYWRIGHT_ENABLE="${"$"}{MCP_PLAYWRIGHT_ENABLE:-0}" node - <<'NODE'
197197
const fs = require("node:fs")
198198
const path = require("node:path")
199199

‎packages/app/src/lib/core/templates-entrypoint/gemini.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,7 @@ fi`
200200
const renderGeminiMcpPlaywrightConfig = (): string =>
201201
String.raw`# Gemini CLI: keep Playwright MCP config in sync with container settings
202202
docker_git_sync_gemini_playwright_mcp() {
203-
GEMINI_CONFIG_SETTINGS_FILE="$GEMINI_CONFIG_SETTINGS_FILE" MCP_PLAYWRIGHT_ENABLE="$MCP_PLAYWRIGHT_ENABLE" node - <<'NODE'
203+
GEMINI_CONFIG_SETTINGS_FILE="$GEMINI_CONFIG_SETTINGS_FILE" MCP_PLAYWRIGHT_ENABLE="${"$"}{MCP_PLAYWRIGHT_ENABLE:-0}" node - <<'NODE'
204204
const fs = require("node:fs")
205205
const path = require("node:path")
206206
const settingsPath = process.env.GEMINI_CONFIG_SETTINGS_FILE

‎packages/app/src/lib/core/templates-entrypoint/grok.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ chmod 0600 "$GROK_CONFIG_SETTINGS_FILE" "$GROK_USER_SETTINGS_FILE" 2>/dev/null |
185185
const renderGrokMcpPlaywrightConfig = (): string =>
186186
String.raw`# Grok CLI: keep Playwright MCP config in sync with container settings
187187
docker_git_sync_grok_playwright_mcp() {
188-
GROK_CONFIG_SETTINGS_FILE="$GROK_CONFIG_SETTINGS_FILE" MCP_PLAYWRIGHT_ENABLE="$MCP_PLAYWRIGHT_ENABLE" node - <<'NODE'
188+
GROK_CONFIG_SETTINGS_FILE="$GROK_CONFIG_SETTINGS_FILE" MCP_PLAYWRIGHT_ENABLE="${"$"}{MCP_PLAYWRIGHT_ENABLE:-0}" node - <<'NODE'
189189
const fs = require("node:fs")
190190
const path = require("node:path")
191191
const settingsPath = process.env.GROK_CONFIG_SETTINGS_FILE

‎packages/app/src/lib/core/templates/dockerfile.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,8 @@ RUN set -eu; \
153153

154154
const dockerGitSessionSyncPackage = "@prover-coder-ai/docker-git-session-sync@latest"
155155

156-
const dockerfilePlaywrightMcpBlock = String.raw`RUN npm install -g @playwright/mcp@latest
156+
const dockerfilePlaywrightMcpBlock = String.raw`ARG PLAYWRIGHT_MCP_VERSION=0.0.75
157+
RUN npm install -g "@playwright/mcp@${"$"}{PLAYWRIGHT_MCP_VERSION}"
157158
158159
# docker-git: wrapper that launches the MCP stdio server without blocking initialize on CDP readiness.
159160
RUN cat <<'EOF' > /usr/local/bin/docker-git-playwright-mcp

‎packages/app/src/lib/core/templates/playwright-browser-runtime.ts‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,16 +48,26 @@ docker_git_wait_for_playwright_cdp() {
4848
local delay="\${MCP_PLAYWRIGHT_READY_DELAY:-1}"
4949
local endpoint
5050
endpoint="$(docker_git_playwright_cdp_endpoint)"
51+
if [[ ! "$attempts" =~ ^[0-9]+$ ]] || (( attempts < 1 )); then
52+
docker_git_browser_log "invalid MCP_PLAYWRIGHT_READY_ATTEMPTS=$attempts; using 60"
53+
attempts=60
54+
fi
55+
if [[ ! "$delay" =~ ^[0-9]+$ ]]; then
56+
docker_git_browser_log "invalid MCP_PLAYWRIGHT_READY_DELAY=$delay; using 1"
57+
delay=1
58+
fi
5159
52-
for attempt in $(seq 1 "$attempts"); do
60+
local attempt=1
61+
while (( attempt <= attempts )); do
5362
if docker_git_fetch_playwright_cdp_version; then
5463
docker_git_browser_log "CDP endpoint is ready: $endpoint"
5564
return 0
5665
fi
57-
if [[ "$attempt" -lt "$attempts" ]]; then
66+
if (( attempt < attempts )); then
5867
docker_git_browser_log "waiting for CDP endpoint $endpoint (attempt $attempt/$attempts)"
5968
sleep "$delay"
6069
fi
70+
attempt=$((attempt + 1))
6171
done
6272
6373
docker_git_browser_log "CDP endpoint did not become ready: $endpoint"

‎packages/app/test-adapters/core-templates.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,2 @@
11
export { defaultTemplateConfig, type TemplateConfig } from "../src/lib/core/domain.js"
22
export { planFiles } from "../src/lib/core/templates.js"
3-
export { renderDockerfile } from "../src/lib/core/templates/dockerfile.js"

‎packages/app/tests/docker-git/core-templates.test.ts‎

Lines changed: 32 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,6 @@
11
import { describe, expect, it } from "@effect/vitest"
22

3-
import {
4-
defaultTemplateConfig,
5-
planFiles,
6-
renderDockerfile,
7-
type TemplateConfig
8-
} from "../../test-adapters/core-templates.js"
3+
import { defaultTemplateConfig, planFiles, type TemplateConfig } from "../../test-adapters/core-templates.js"
94

105
const makeTemplateConfig = (overrides: Partial<TemplateConfig> = {}): TemplateConfig => ({
116
...defaultTemplateConfig,
@@ -30,28 +25,45 @@ const makeTemplateConfig = (overrides: Partial<TemplateConfig> = {}): TemplateCo
3025
...overrides
3126
})
3227

28+
type PlannedFile = ReturnType<typeof planFiles>[number]
29+
type GeneratedFile = Extract<PlannedFile, { readonly _tag: "File" }>
30+
31+
const getGeneratedFile = (files: ReadonlyArray<PlannedFile>, relativePath: string): GeneratedFile => {
32+
const file = files.find(
33+
(candidate): candidate is GeneratedFile => candidate._tag === "File" && candidate.relativePath === relativePath
34+
)
35+
if (file === undefined) {
36+
throw new Error(`Missing generated file: ${relativePath}`)
37+
}
38+
return file
39+
}
40+
41+
const getGeneratedFilePaths = (files: ReadonlyArray<PlannedFile>): ReadonlyArray<string> =>
42+
files.flatMap((file) => file._tag === "File" ? [file.relativePath] : [])
43+
3344
describe("app planFiles", () => {
3445
it("includes nested browser runtime artifacts when Playwright is enabled", () => {
3546
const files = planFiles(makeTemplateConfig({ enableMcpPlaywright: true }))
36-
const filePaths = files.flatMap((file) => file._tag === "File" ? [file.relativePath] : [])
37-
const runtime = files.find(
38-
(file): file is Extract<(typeof files)[number], { readonly _tag: "File" }> =>
39-
file._tag === "File" && file.relativePath === "docker-git-browser-runtime.sh"
40-
)
41-
const dockerfile = renderDockerfile(makeTemplateConfig({ enableMcpPlaywright: true }))
47+
const filePaths = getGeneratedFilePaths(files)
48+
const runtime = getGeneratedFile(files, "docker-git-browser-runtime.sh")
49+
const dockerfile = getGeneratedFile(files, "Dockerfile")
4250

4351
expect(filePaths).toContain("Dockerfile.browser")
4452
expect(filePaths).toContain("mcp-playwright-start-extra.sh")
4553
expect(filePaths).toContain("docker-git-browser-runtime.sh")
46-
expect(runtime).toBeDefined()
47-
expect(runtime?.mode).toBe(0o755)
48-
expect(runtime?.contents).toContain("if [[ \"${MCP_PLAYWRIGHT_ENABLE:-0}\" != \"1\" ]]; then")
49-
expect(runtime?.contents).toContain("docker_git_wait_for_playwright_cdp()")
50-
expect(runtime?.contents).toContain("MCP_PLAYWRIGHT_ENABLE=0")
51-
expect(runtime?.contents).not.toContain("\\${MCP_PLAYWRIGHT_ENABLE:-0}")
52-
expect(dockerfile).toContain(
54+
expect(runtime.mode).toBe(0o755)
55+
expect(runtime.contents).toContain("if [[ \"${MCP_PLAYWRIGHT_ENABLE:-0}\" != \"1\" ]]; then")
56+
expect(runtime.contents).toContain("docker_git_wait_for_playwright_cdp()")
57+
expect(runtime.contents).toContain("MCP_PLAYWRIGHT_ENABLE=0")
58+
expect(runtime.contents).not.toContain("\\${MCP_PLAYWRIGHT_ENABLE:-0}")
59+
expect(dockerfile.contents).toContain(
5360
"COPY Dockerfile.browser mcp-playwright-start-extra.sh docker-git-browser-runtime.sh /opt/docker-git/browser/"
5461
)
55-
expect(dockerfile).toContain("MCP_PLAYWRIGHT_CDP_TIMEOUT=\"${MCP_PLAYWRIGHT_CDP_TIMEOUT:-60000}\"")
62+
expect(dockerfile.contents).toContain("ARG PLAYWRIGHT_MCP_VERSION=0.0.75")
63+
expect(dockerfile.contents).toContain("RUN npm install -g \"@playwright/mcp@${PLAYWRIGHT_MCP_VERSION}\"")
64+
expect(dockerfile.contents).toContain("MCP_PLAYWRIGHT_CDP_TIMEOUT=\"${MCP_PLAYWRIGHT_CDP_TIMEOUT:-60000}\"")
65+
expect(runtime.contents).toContain("invalid MCP_PLAYWRIGHT_READY_ATTEMPTS")
66+
expect(runtime.contents).toContain("while (( attempt <= attempts )); do")
67+
expect(runtime.contents).not.toContain("for attempt in $(seq 1 \"$attempts\")")
5668
})
5769
})

‎packages/lib/src/core/templates-entrypoint/claude.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,7 @@ const renderClaudeMcpPlaywrightConfig = (): string =>
192192
String.raw`# Claude Code: keep Playwright MCP config in sync with container settings
193193
CLAUDE_SETTINGS_FILE="${"$"}{CLAUDE_HOME_JSON:-$CLAUDE_CONFIG_DIR/.claude.json}"
194194
docker_git_sync_claude_playwright_mcp() {
195-
CLAUDE_SETTINGS_FILE="$CLAUDE_SETTINGS_FILE" MCP_PLAYWRIGHT_ENABLE="$MCP_PLAYWRIGHT_ENABLE" node - <<'NODE'
195+
CLAUDE_SETTINGS_FILE="$CLAUDE_SETTINGS_FILE" MCP_PLAYWRIGHT_ENABLE="${"$"}{MCP_PLAYWRIGHT_ENABLE:-0}" node - <<'NODE'
196196
const fs = require("node:fs")
197197
const path = require("node:path")
198198

‎packages/lib/src/core/templates-entrypoint/gemini.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,7 @@ fi`
199199
const renderGeminiMcpPlaywrightConfig = (): string =>
200200
String.raw`# Gemini CLI: keep Playwright MCP config in sync with container settings
201201
docker_git_sync_gemini_playwright_mcp() {
202-
GEMINI_CONFIG_SETTINGS_FILE="$GEMINI_CONFIG_SETTINGS_FILE" MCP_PLAYWRIGHT_ENABLE="$MCP_PLAYWRIGHT_ENABLE" node - <<'NODE'
202+
GEMINI_CONFIG_SETTINGS_FILE="$GEMINI_CONFIG_SETTINGS_FILE" MCP_PLAYWRIGHT_ENABLE="${"$"}{MCP_PLAYWRIGHT_ENABLE:-0}" node - <<'NODE'
203203
const fs = require("node:fs")
204204
const path = require("node:path")
205205
const settingsPath = process.env.GEMINI_CONFIG_SETTINGS_FILE

‎packages/lib/src/core/templates-entrypoint/grok.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,7 @@ chmod 0600 "$GROK_CONFIG_SETTINGS_FILE" "$GROK_USER_SETTINGS_FILE" 2>/dev/null |
184184
const renderGrokMcpPlaywrightConfig = (): string =>
185185
String.raw`# Grok CLI: keep Playwright MCP config in sync with container settings
186186
docker_git_sync_grok_playwright_mcp() {
187-
GROK_CONFIG_SETTINGS_FILE="$GROK_CONFIG_SETTINGS_FILE" MCP_PLAYWRIGHT_ENABLE="$MCP_PLAYWRIGHT_ENABLE" node - <<'NODE'
187+
GROK_CONFIG_SETTINGS_FILE="$GROK_CONFIG_SETTINGS_FILE" MCP_PLAYWRIGHT_ENABLE="${"$"}{MCP_PLAYWRIGHT_ENABLE:-0}" node - <<'NODE'
188188
const fs = require("node:fs")
189189
const path = require("node:path")
190190
const settingsPath = process.env.GROK_CONFIG_SETTINGS_FILE

0 commit comments

Comments
 (0)