-
Notifications
You must be signed in to change notification settings - Fork 578
fix(docker): Pre-install Playwright Chromium browsers for automated t… #745
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
45f6f17
b37a287
3ccea7a
d4439fa
8226699
a08ba1b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,4 +96,4 @@ data/credentials.json | |
| data/ | ||
| .codex/ | ||
| .mcp.json | ||
| .planning | ||
| .planning | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -322,11 +322,12 @@ export function createVerifyClaudeAuthHandler() { | |
| }); | ||
|
|
||
| // Determine specific auth type for success messages | ||
| const effectiveAuthMethod = authMethod ?? 'api_key'; | ||
| let authType: 'oauth' | 'api_key' | 'cli' | undefined; | ||
| if (authenticated) { | ||
| if (authMethod === 'api_key') { | ||
| if (effectiveAuthMethod === 'api_key') { | ||
| authType = 'api_key'; | ||
| } else if (authMethod === 'cli') { | ||
| } else if (effectiveAuthMethod === 'cli') { | ||
| // Check if CLI auth is via OAuth (Claude Code subscription) or generic CLI | ||
| try { | ||
| const indicators = await getClaudeAuthIndicators(); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note Nit - Performance: Redundant filesystem checks after successful auth verification After successfully verifying authentication via a full SDK query (which already proved auth works), this code calls Problematic code: const indicators = await getClaudeAuthIndicators();Suggested fix: This is acceptable for now since it only runs during setup verification (not a hot path). Could be improved by caching the indicators or passing them through context, but low priority. |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Minor - Logic: Misleading error handling in Playwright install RUN command
The
|| echoat the end is connected to thelscommand due to operator precedence, not to the entire chain. Ifplaywright install chromiumfails, the build will fail (which is correct). However, iflsfails (unlikely but possible), theechosilently masks the error. This creates a misleading success message. More importantly, the||structure makes the overall RUN step always succeed even iflsfails, which could hide a misconfigured browser install path.Problematic code:
Suggested fix:
Use explicit grouping or just remove the fallback since the ls output is informational: ```dockerfile RUN ./node_modules/.bin/playwright install chromium && \ echo "=== Playwright Chromium installed ===" && \ ls -la /home/automaker/.cache/ms-playwright/