fix: ngrok error logging by adding real-time output#51
Open
yCodeTech wants to merge 8 commits into
Open
Conversation
It was really hard to figure out if there was something wrong with ngrok without proper logging. Ngrok does actually have a --log option. so we can use this to stream live logging output directly to the terminal. - Implemented `streamCommandOutput` method in `CommandLine` to handle command output in real time, while also optionally collecting errors for later checks, and format errors as valet errors. - Enhanced `Ngrok` class to utilize the new streaming method with the --log options for better error handling and output visibility. - Updated `error` helper function to support additional `newline` and `escapeOutput` parameters for output formatting, and changed the Symfony's `writeln` to `write` so we can toggle on/off the writing of a newline at the end of the output.
- Updated `output` helper function to accept a `newline` parameter, and changed the Symfony's `writeln` to `write` so we can toggle on/off the writing of a newline at the end of the output.
- Refactored `CommandLine::streamCommandOutput` method:
- Removed the callback `lineMatches` and `lineIsError` params.
- Added a `callbacks` array param to define the many callbacks that may be used, for better flexibility.
- Updated the callback defaults.
- Updated ngrok integration to utilize the new callback structure for error line handling.
…sers
- Refactored the host-header options conditional check in favour of defining the default options including the log options and their values in an array, and looping through them only adding them if they're not in the user options array.
- Allowed the logging options to be overridden by users by removing the hardcoded logging options from the ngrok command in favour of the options array.
- Added a `lineHandler` callback option to the `CommandLine::streamCommandOutput` method, and allow it to be called after the line has been outputted to the terminal. - Added a line handler closure function in `Ngrok::start` method to find and extract the public URL when the tunnel starts and output it as a Valet info message, so the user can easily identify the URL without having to use the `fetch-share-url` valet command. - Added a conditional to detect if the options string doesn't contain the log stdout/stderr option, then it'll output the info message to inform users they can use the `fetch-share-url` valet command. This message doesn't need to be outputted if it is logging to stdout/stderr as it tells the use about the url anyway. - Also added a notes about why stdout and stderr values are the same - because `CommandLine::streamCommandOutput` method uses `2>&1` to redirect stderr to stdout.
…grok ngrok's command output is now processed through the `output` helper function which uses Symfony's `ConsoleOutput` instead of relying on PHP's `echo`. This fixes the output race where the URL info in the lineHandler was outputted before ngrok's own output was shown. By changing it to our helper, everything goes through Symfony's buffer system and output them in order.
- Implemented `ShareTool::copyUrlToClipboard` method to copy the specified URL to the clipboard, and changed the `ShareTool::currentTunnelUrl` method to use this new method instead of duplicating the code. - Removed the info message from the `fetch-share-url` command, and added it into the new `ShareTool::copyUrlToClipboard` method. - Updated `Ngrok:start` method to use the new `ShareTool::copyUrlToClipboard` method for copying the URL in the `lineHandler`.
There was a problem hiding this comment.
Pull request overview
This PR improves the ngrok share experience by streaming ngrok logs to the terminal in real time (instead of only capturing post-run output), while also centralizing clipboard-copy behavior for the public tunnel URL.
Changes:
- Added
CommandLine::streamCommandOutput()to stream process output line-by-line and optionally capture matching lines for later analysis. - Updated
Ngrok::start()to run ngrok with stdout logging enabled by default, stream output live, and extract/copy the public URL when the tunnel starts. - Refactored share URL clipboard copy messaging by moving it into
ShareTool::copyUrlToClipboard()and updating console helpers to support newline/escaping behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cli/Valet/ShareTools/ShareTool.php | Adds a dedicated copyUrlToClipboard() helper and routes URL copying through it. |
| cli/Valet/ShareTools/Ngrok.php | Streams ngrok output live, applies default --log* flags, parses the “started tunnel” URL, and performs post-run error checks. |
| cli/Valet/CommandLine.php | Introduces streamCommandOutput() for real-time output streaming and optional line capture. |
| cli/valet.php | Removes duplicated “copied to clipboard” messaging since it’s now handled in ShareTool. |
| cli/includes/helpers.php | Extends error()/output() helpers to support newline control and optional escaping. |
Comment on lines
+141
to
+142
| $this->cli->passthru("echo $url | clip"); | ||
| info("It has been copied to your clipboard."); |
Comment on lines
+68
to
+70
| $isErrorLine = function ($line) { | ||
| return strpos($line, 'ERROR:') !== false; | ||
| }; |
Comment on lines
+50
to
+51
| $handle = popen("$command 2>&1", 'r'); | ||
| while ($handle && !feof($handle)) { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Technically a follow-up to issue #48 that prompted me to double check ngrok was running properly; and also a follow-up and enhancement of commit 2724314
It was really hard to figure out if there was something wrong with ngrok without proper logging. Ngrok does actually have a --log option. so we can use this to stream live logging output directly to the terminal.
Implemented
streamCommandOutputmethod inCommandLineto handle command output in real time, while also optionally collecting errors for later checks, and format errors as valet errors.Enhanced
Ngrokclass to utilize the new streaming method with the --log options for better error handling and output visibility.Updated
errorhelper function to support additionalnewlineandescapeOutputparameters for output formatting, and changed the Symfony'swritelntowriteso we can toggle on/off the writing of a newline at the end of the output.