fix: run playwright preflight via current python#281
fix: run playwright preflight via current python#281voidborne-d wants to merge 1 commit intoteng-lin:mainfrom
Conversation
📝 WalkthroughWalkthroughUpdated Playwright invocation to use Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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 |
![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 browser installation logic to use the current Python interpreter via sys.executable -m playwright, ensuring compatibility with virtual environments and pipx. It also includes new unit tests to verify the command execution. The review feedback suggests improving the user experience by allowing installation progress to be visible in the console and ensuring the Python executable path is quoted in error messages to handle paths containing spaces.
| install_result = subprocess.run( | ||
| ["playwright", "install", "chromium"], | ||
| [*playwright_cmd, "install", "chromium"], | ||
| capture_output=True, | ||
| text=True, | ||
| ) |
There was a problem hiding this comment.
Capturing output for the actual installation command prevents the user from seeing the progress bar and any detailed error messages from Playwright. Since this command can take a significant amount of time to download the browser binaries, it's better to let the output flow to the console so the user can monitor the progress.
install_result = subprocess.run(
[*playwright_cmd, "install", "chromium"],
)| 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.
If the path to the Python executable contains spaces (which is common on Windows, e.g., in C:\Program Files\...), the suggested manual command will fail when copy-pasted. Wrapping the executable path in quotes ensures it remains a single argument.
| f"Run manually: {sys.executable} -m playwright install chromium" | |
| f"Run manually: \"{sys.executable}\" -m playwright install chromium" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/cli/test_session.py (1)
85-99: Consider asserting the full call sequence in the install-path test.Right now the second test checks only positional argv content. Asserting the exact two calls (including kwargs) would catch regressions in invocation flags too.
Proposed tightening for this test
-from unittest.mock import AsyncMock, MagicMock, patch +from unittest.mock import AsyncMock, MagicMock, call, patch @@ - assert mock_run.call_args_list[0].args[0] == [ - sys.executable, - "-m", - "playwright", - "install", - "--dry-run", - "chromium", - ] - assert mock_run.call_args_list[1].args[0] == [ - sys.executable, - "-m", - "playwright", - "install", - "chromium", - ] + assert mock_run.call_args_list == [ + call( + [sys.executable, "-m", "playwright", "install", "--dry-run", "chromium"], + capture_output=True, + text=True, + ), + call( + [sys.executable, "-m", "playwright", "install", "chromium"], + capture_output=True, + text=True, + ), + ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/cli/test_session.py` around lines 85 - 99, Update the install-path test to assert the complete sequence of mock_run calls (including kwargs) instead of only positional argv for the second call: inspect mock_run.call_args_list and compare both entries to the exact expected call tuples/dicts so you verify both args and kwargs; use the existing mock_run reference and assert mock_run.call_args_list == [call([...], **{...}), call([...], **{...})] (or equivalent explicit comparisons) to lock down invocation flags and prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/cli/test_session.py`:
- Around line 85-99: Update the install-path test to assert the complete
sequence of mock_run calls (including kwargs) instead of only positional argv
for the second call: inspect mock_run.call_args_list and compare both entries to
the exact expected call tuples/dicts so you verify both args and kwargs; use the
existing mock_run reference and assert mock_run.call_args_list == [call([...],
**{...}), call([...], **{...})] (or equivalent explicit comparisons) to lock
down invocation flags and prevent regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d95fd19d-5c2a-4b79-bbe1-075da50eade0
📒 Files selected for processing (2)
src/notebooklm/cli/session.pytests/unit/cli/test_session.py
1 participant
Summary
Testing
68 passed in 0.38s
Closes #278.
Summary by CodeRabbit
Bug Fixes
Tests