fix(browser_profiler): cross-platform 'q' to quit - create profile #1170
Conversation
This commit introduces platform-specific handling for the 'q' key press to quit the browser profiler, ensuring compatibility with both Windows and Unix-like systems. It also adds a check to see if the browser process has already exited, terminating the input listener if so. - Implemented `msvcrt` for Windows to capture keyboard input without requiring a newline. - Retained `termios`, `tty`, and `select` for Unix-like systems. - Added a check for browser process termination to gracefully exit the input listener. - Updated logger messages to use colored output for better user experience.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe code refactors the asynchronous keyboard input listener in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Listener (Unix/Win)
participant BrowserProcess
Note over Listener (Unix/Win): Start async input listener
loop Until 'q' pressed or browser exits
Listener (Unix/Win) -> Listener (Unix/Win): Check for keypress ('q')
Listener (Unix/Win) -> BrowserProcess: Check if browser exited
BrowserProcess --> Listener (Unix/Win): Browser running/exited
alt 'q' pressed or browser exited
Listener (Unix/Win) -> Listener (Unix/Win): Exit input loop
end
end
Listener (Unix/Win) -> Listener (Unix/Win): Cleanup (restore terminal if Unix)
Assessment against linked issues
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crawl4ai/browser_profiler.py (2)
240-257: Avoid blocking the event loop withselect.selectin tight async loops
select.select(..., timeout=0.5)blocks the entire asyncio event loop for up to half a second on every iteration, affecting concurrent tasks (e.g., network I/O in the crawler).Consider running the blocking stdin poll in a background thread:
loop = asyncio.get_running_loop() key = await loop.run_in_executor(None, sys.stdin.read, 1)or off-loading the whole listener with
asyncio.to_thread.This keeps the main loop responsive without sacrificing portability.
Also applies to: 738-749
183-224: DRY: extract the nearly identical quit listeners into a reusable helperThe two
listen_for_quit_commandcoroutines are almost identical (only log tags differ). Duplicating ~90 lines inflates maintenance cost and risks future divergence.Refactor into one parametric helper, e.g.:
async def _await_quit_or_exit( tag: str, logger: AsyncLoggerBase, browser_proc: subprocess.Popen, done_evt: asyncio.Event, ): ...Both call-sites can then
await _await_quit_or_exit("PROFILE", self.logger, browser_process, user_done_event).Also applies to: 695-726
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crawl4ai/browser_profiler.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
crawl4ai/browser_profiler.py (1)
crawl4ai/async_logger.py (4)
info(55-56)info(236-238)info(349-351)LogColor(29-46)
|
@ntohidi Would you like us to go ahead and make these changes? I can prep the update if you're good with it. |
@prokopis3 I believe it's worthwhile to consider this. Please proceed. |
- fix handling of special keys in Windows msvcrt implementation - Guard against UnicodeDecodeError from multi-byte key sequences - Filter out non-printable characters and control sequences - Add error handling to prevent coroutine crashes - Add unit test to verify keyboard input handling Key changes: - Safe UTF-8 decoding with try/except for special keys - Skip non-printable and multi-byte character sequences - Add broad exception handling in keyboard listener Test runs on Windows only due to msvcrt dependency.
…st_create_profile.py ) - Rename file to correct spelling - No content changes
|
@ntohidi done! |
Summary
This PR implements cross-platform keyboard input handling for the browser profiler's quit command, addressing platform compatibility issues between Windows and Unix-like systems.
msvcrtfor Windows to capture keyboard input without requiring a newline.termios,tty, andselectfor Unix-like systems.Fixes #1166
List of files changed and why
browser_profiler.pymsvcrtfor Windowstermios,tty, andselectfor Unix systemsHow Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist:
Summary by CodeRabbit