fix(login): use sys.executable for Playwright subprocess calls#279
fix(login): use sys.executable for Playwright subprocess calls#279shidoyu wants to merge 2 commits intoteng-lin:mainfrom
Conversation
Replace bare `playwright` command in `_ensure_chromium_installed()` with `sys.executable -m playwright` to ensure the correct Python environment's Playwright is invoked. This fixes pre-flight checks failing silently when the `playwright` CLI entry point is not on PATH, which is common in virtualenvs, pipx installs, and embedded Python environments. Changes: - Use `[sys.executable, "-m", "playwright", ...]` for dry-run check - Use `[sys.executable, "-m", "playwright", ...]` for auto-install - Update manual install instruction in error message to match
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Playwright installation logic to use the current Python executable (sys.executable -m playwright) instead of the global command, ensuring the correct environment is used. A review comment suggests wrapping the executable path in quotes within the manual installation error message to properly handle file paths that contain spaces.
| console.print( | ||
| "[red]Failed to install Chromium browser.[/red]\n" | ||
| "Run manually: playwright install chromium" | ||
| f"Run manually: {sys.executable} -m playwright install chromium" |
There was a problem hiding this comment.
When sys.executable contains spaces (which is common on Windows, e.g., in C:\Program Files\...), the suggested manual command will fail if copy-pasted directly into a terminal. It is safer to wrap the executable path in quotes to ensure it is treated as a single argument.
| f"Run manually: {sys.executable} -m playwright install chromium" | |
| f'Run manually: "{sys.executable}" -m playwright install chromium' |
📝 WalkthroughWalkthroughChromium pre-flight install in the session CLI now runs Playwright via the current Python interpreter using Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/notebooklm/cli/session.py (1)
311-323: Optional: deduplicate Playwright command construction to avoid drift.You build the same command prefix twice. Extracting it once reduces future mismatch risk between dry-run/install paths.
Suggested refactor
@@ - result = subprocess.run( - [sys.executable, "-m", "playwright", "install", "--dry-run", "chromium"], + playwright_install_cmd = [sys.executable, "-m", "playwright", "install"] + result = subprocess.run( + [*playwright_install_cmd, "--dry-run", "chromium"], capture_output=True, text=True, ) @@ - install_result = subprocess.run( - [sys.executable, "-m", "playwright", "install", "chromium"], + install_result = subprocess.run( + [*playwright_install_cmd, "chromium"], capture_output=True, text=True, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooklm/cli/session.py` around lines 311 - 323, The Playwright install logic duplicates the command construction; refactor in session.py by extracting the shared command prefix (e.g., a variable like playwright_install_cmd = [sys.executable, "-m", "playwright", "install"]) and then reuse it for the dry-run and the real install calls by appending the mode and target (e.g., + ["--dry-run", "chromium"] or + ["chromium"]) when calling subprocess.run in the surrounding function/block, ensuring both subprocess.run invocations use the same base command to avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/notebooklm/cli/session.py`:
- Around line 311-323: The Playwright install logic duplicates the command
construction; refactor in session.py by extracting the shared command prefix
(e.g., a variable like playwright_install_cmd = [sys.executable, "-m",
"playwright", "install"]) and then reuse it for the dry-run and the real install
calls by appending the mode and target (e.g., + ["--dry-run", "chromium"] or +
["chromium"]) when calling subprocess.run in the surrounding function/block,
ensuring both subprocess.run invocations use the same base command to avoid
drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7707b9f-6efc-47ad-a88b-83e989aafbe8
📒 Files selected for processing (1)
src/notebooklm/cli/session.py
Paths like `/Users/john doe/.venv/bin/python3` need quoting in the manual install instruction to be safely copy-pasted into a shell.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/notebooklm/cli/session.py (1)
329-329: Keep install guidance consistent across this module.Nice improvement on this manual command. There is still a bare
playwright install chromiumhint in theImportErrorbranch at Line 422; that path can still hit the same PATH issue. Consider switching that hint to the same"{sys.executable}" -m playwright ...form.Suggested follow-up diff
- install_hint = " pip install notebooklm[browser]\n playwright install chromium" + install_hint = ( + f" pip install notebooklm[browser]\n" + f' "{sys.executable}" -m playwright install chromium' + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooklm/cli/session.py` at line 329, Update the ImportError branch that suggests installing Playwright so it uses the same sys.executable invocation as the other branch; specifically, replace the bare "playwright install chromium" hint in the ImportError handling code (the message produced in the except ImportError block in session.py) with the formatted command f'"{sys.executable}" -m playwright install chromium' so both branches offer the consistent, PATH-safe install instruction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/notebooklm/cli/session.py`:
- Line 329: Update the ImportError branch that suggests installing Playwright so
it uses the same sys.executable invocation as the other branch; specifically,
replace the bare "playwright install chromium" hint in the ImportError handling
code (the message produced in the except ImportError block in session.py) with
the formatted command f'"{sys.executable}" -m playwright install chromium' so
both branches offer the consistent, PATH-safe install instruction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a0b8022-4daf-40f3-99e6-4d5fd11b396e
📒 Files selected for processing (1)
src/notebooklm/cli/session.py
1 participant
Summary
Replace bare
playwrightcommand in_ensure_chromium_installed()withsys.executable -m playwrightto ensure the correct Python environment's Playwright is invoked.Fixes #278
Changes
[sys.executable, "-m", "playwright", ...]for the dry-run Chromium check[sys.executable, "-m", "playwright", ...]for the auto-install commandWhy
When notebooklm-py is installed in a virtualenv or via pipx, the
playwrightCLI entry point may not be on$PATH. The current code callssubprocess.run(["playwright", ...]), which raisesFileNotFoundError— caught silently, but avoidable.Using
sys.executable -m playwrightis the standard Python pattern (used by pip, pytest, etc.) and guarantees the subprocess uses the same Python interpreter that's running the package.Related Issue
#278
Test Plan
ruff checkpassesruff format --checkpassesmypy(running locally)pytest(running locally)Manual test
Verified on macOS 15.3, Python 3.12 in a virtualenv where
playwrightis not on PATH:Warning: Chromium pre-flight check failed: [Errno 2] No such file or directory: 'playwright'Summary by CodeRabbit