#272: created the Javascript trait for tracking js errors on the page #418
Conversation
WalkthroughA new PHP class, Changes
Poem
✨ Finishing Touches
🪧 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/JavascriptErrorContext.php (2)
58-74: Likewise, markafterStep()as a Behat hookAdd
@AfterStepso the collection logic is actually triggered.- public function afterStep(AfterStepScope $scope): void { + /** + * @AfterStep + */ + public function afterStep(AfterStepScope $scope): void {
95-108: Scenario hook also needs registrationSame reasoning – add the hook annotation/attribute:
- public function afterScenario(AfterScenarioScope $scope): void { + /** + * @AfterScenario + */ + public function afterScenario(AfterScenarioScope $scope): void {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/JavascriptErrorContext.php(1 hunks)tests/behat/features/js_error.feature(1 hunks)
🧰 Additional context used
🪛 PHPStan (2.1.15)
src/JavascriptErrorContext.php
21-21: Property DrevOps\BehatSteps\JavascriptErrorContext::$jsErrorRegistry type has no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
(missingType.iterableValue)
26-26: Property DrevOps\BehatSteps\JavascriptErrorContext::$stepFilters type has no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
(missingType.iterableValue)
| public function beforeStep(BeforeStepScope $scope): void { | ||
| $stepText = $scope->getStep()->getText(); | ||
| if ($this->shouldCaptureForStep($stepText)) { | ||
| $script = <<<JS | ||
| if (!window.__behatErrors) { | ||
| window.__behatErrors = []; | ||
| (function() { | ||
| const originalError = console.error; | ||
| console.error = function(...args) { | ||
| window.__behatErrors.push(args.join(" ")); | ||
| originalError.apply(console, args); | ||
| }; | ||
| })(); | ||
| } | ||
| JS; | ||
| $this->getSession()->executeScript($script); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Behat hooks will not execute – missing @BeforeStep annotation/attribute
beforeStep() is intended to run as a Behat hook, but Behat only registers hooks that are explicitly marked with either a PHP-8 attribute (#[BeforeStep]) or a doc-block annotation (@BeforeStep).
Without one of those markers the method is treated as an ordinary helper and will never be called, so no script is injected and no errors are captured.
- public function beforeStep(BeforeStepScope $scope): void {
+ /**
+ * @BeforeStep
+ */
+ public function beforeStep(BeforeStepScope $scope): void {(Repeat the same fix with @AfterStep for afterStep() and @AfterScenario for afterScenario().)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function beforeStep(BeforeStepScope $scope): void { | |
| $stepText = $scope->getStep()->getText(); | |
| if ($this->shouldCaptureForStep($stepText)) { | |
| $script = <<<JS | |
| if (!window.__behatErrors) { | |
| window.__behatErrors = []; | |
| (function() { | |
| const originalError = console.error; | |
| console.error = function(...args) { | |
| window.__behatErrors.push(args.join(" ")); | |
| originalError.apply(console, args); | |
| }; | |
| })(); | |
| } | |
| JS; | |
| $this->getSession()->executeScript($script); | |
| } | |
| } | |
| /** | |
| * @BeforeStep | |
| */ | |
| public function beforeStep(BeforeStepScope $scope): void { | |
| $stepText = $scope->getStep()->getText(); | |
| if ($this->shouldCaptureForStep($stepText)) { | |
| $script = <<<JS | |
| if (!window.__behatErrors) { | |
| window.__behatErrors = []; | |
| (function() { | |
| const originalError = console.error; | |
| console.error = function(...args) { | |
| window.__behatErrors.push(args.join(" ")); | |
| originalError.apply(console, args); | |
| }; | |
| })(); | |
| } | |
| JS; | |
| $this->getSession()->executeScript($script); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/JavascriptErrorContext.php around lines 35 to 52, the beforeStep() method
lacks the necessary Behat hook annotation or attribute, so it will not be
executed as a hook. Add the PHP-8 attribute #[BeforeStep] or the doc-block
annotation @BeforeStep immediately above the beforeStep() method declaration.
Similarly, add #[AfterStep] or @AfterStep above afterStep() and #[AfterScenario]
or @AfterScenario above afterScenario() to ensure these methods are registered
and executed as Behat hooks.
| /** | ||
| * Errors registry keyed by path. | ||
| */ | ||
| private array $jsErrorRegistry = []; | ||
|
|
||
| /** | ||
| * Step patterns. | ||
| */ | ||
| private array $stepFilters = [ | ||
| '/^I (visit|go to)/', | ||
| ]; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Silence PHPStan “no value type specified” warnings
Add generics in doc-blocks so static analysis knows the structure of the arrays:
- private array $jsErrorRegistry = [];
+ /** @var array<string, string[]> */
+ private array $jsErrorRegistry = [];
- private array $stepFilters = [
+ /** @var array<int, string> */
+ private array $stepFilters = [Purely cosmetic but keeps PHPStan quiet and improves IDE tooling.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Errors registry keyed by path. | |
| */ | |
| private array $jsErrorRegistry = []; | |
| /** | |
| * Step patterns. | |
| */ | |
| private array $stepFilters = [ | |
| '/^I (visit|go to)/', | |
| ]; | |
| /** | |
| * Errors registry keyed by path. | |
| */ | |
| /** @var array<string, string[]> */ | |
| private array $jsErrorRegistry = []; | |
| /** | |
| * Step patterns. | |
| */ | |
| /** @var array<int, string> */ | |
| private array $stepFilters = [ | |
| '/^I (visit|go to)/', | |
| ]; |
🧰 Tools
🪛 PHPStan (2.1.15)
21-21: Property DrevOps\BehatSteps\JavascriptErrorContext::$jsErrorRegistry type has no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
(missingType.iterableValue)
26-26: Property DrevOps\BehatSteps\JavascriptErrorContext::$stepFilters type has no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
(missingType.iterableValue)
🤖 Prompt for AI Agents
In src/JavascriptErrorContext.php around lines 18 to 29, the private arrays
$jsErrorRegistry and $stepFilters lack generic type annotations in their
doc-blocks, causing PHPStan to emit "no value type specified" warnings. Add
appropriate generic type annotations in the doc-block comments above these
properties to specify the expected types of the array keys and values, which
will silence PHPStan warnings and improve IDE support.
| private array $stepFilters = [ | ||
| '/^I (visit|go to)/', | ||
| ]; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Navigation pattern "I am on" isn’t captured
The first step in your feature uses “Given I am on …”, yet stepFilters only checks for "I visit" and "I go to".
Consequently the very first page load skips JS-error tracking.
private array $stepFilters = [
'/^I (visit|go to)/',
+ '/^I am on/',
];Consider also exposing the default filters as constants or allowing configuration so projects with different vocabularies don’t have to fork the class.
I’ll verify the patterns and class configuration support by inspecting the stepFilters definition and related methods.
🏁 Script executed:
#!/bin/bash
# Locate the file
file=$(fd JavascriptErrorContext.php)
echo "Inspecting: $file"
# Show where stepFilters is declared
grep -n "stepFilters" -n "$file"
# Display the first 200 lines to review definition and constructor
sed -n '1,200p' "$file"Length of output: 3779
Add missing step filter for “I am on” navigation pattern
The stepFilters array currently only matches I visit and I go to, so steps like “I am on …” aren’t captured and the initial page load skips JS-error tracking. Update the filter definition as follows:
private array $stepFilters = [
- '/^I (visit|go to)/',
+ '/^I (visit|go to)/',
+ '/^I am on/',
];Additional recommendations:
- Expose the default filters as a class constant or accept them via constructor/configuration to avoid forking when projects use different step vocabularies.
- Projects can still add custom patterns at runtime using
addStepFilter(), but making the defaults configurable improves flexibility.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private array $stepFilters = [ | |
| '/^I (visit|go to)/', | |
| ]; | |
| private array $stepFilters = [ | |
| '/^I (visit|go to)/', | |
| '/^I am on/', | |
| ]; |
🧰 Tools
🪛 PHPStan (2.1.15)
26-26: Property DrevOps\BehatSteps\JavascriptErrorContext::$stepFilters type has no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
(missingType.iterableValue)
🤖 Prompt for AI Agents
In src/JavascriptErrorContext.php around lines 26 to 29, the stepFilters array
only matches "I visit" and "I go to" patterns, missing the "I am on" navigation
phrase, causing initial page loads to skip JS-error tracking. Update the
stepFilters array to include a pattern matching "I am on" steps. Additionally,
refactor the class to expose the default filters as a class constant or accept
them via constructor or configuration, allowing projects to customize or extend
step filters without forking the class.
| $script = <<<JS | ||
| if (!window.__behatErrors) { | ||
| window.__behatErrors = []; | ||
| (function() { | ||
| const originalError = console.error; | ||
| console.error = function(...args) { | ||
| window.__behatErrors.push(args.join(" ")); | ||
| originalError.apply(console, args); | ||
| }; | ||
| })(); | ||
| } | ||
| JS; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Injected logger is wiped out by page reloads
console.error is overridden before the navigation step executes.
When the browser navigates, the page context (and thus the override) is lost, meaning errors on the new page won’t be recorded.
Two possible fixes:
- Inject the logger after the navigation completes (inside
afterStep()before you read the buffer for the next step). - Re-inject the script immediately after you clear the buffer in
afterStep().
Please verify the current behaviour with a page that throws a deliberate console.error on load; you’ll see it is not captured.
🏁 Script executed:
#!/usr/bin/env bash
set -eux
# Locate the JavascriptErrorContext file
file=$(fd JavascriptErrorContext.php)
# Display the file path
echo "Found file: $file"
# Show the first 150 lines for context
sed -n '1,150p' "$file"
# Search for hook annotations to see when injection happens
rg -n '@BeforeStep|@BeforeScenario|@AfterStep' "$file" || trueLength of output: 3693
Ensure the JS error logger is re-injected after each navigation
Overriding console.error in beforeStep() happens before the visit step runs, so the override is lost when the browser reloads into the new page context. You must inject (or re-inject) the logger after navigation completes.
Files to update:
src/JavascriptErrorContext.php- beforeStep() is too early for navigation steps.
- afterStep() needs to re-inject the override for navigation steps.
Suggested change in afterStep() (inside the if ($this->shouldCaptureForStep(...)) block):
// Clear the error buffer to avoid duplicate capture
- $this->getSession()->executeScript('window.__behatErrors = [];');
+ $this->getSession()->executeScript('window.__behatErrors = [];');
+
+ // Re-inject the error logger into the new page context
+ $this->getSession()->executeScript(<<<'JS'
+ // Initialize buffer and override console.error
+ window.__behatErrors = [];
+ (function() {
+ const originalError = console.error;
+ console.error = function(...args) {
+ window.__behatErrors.push(args.join(" "));
+ originalError.apply(console, args);
+ };
+ })();
+ JS
+ );After applying, verify by visiting a page that calls console.error on load—you should now capture that error.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/JavascriptErrorContext.php between lines 38 and 49, the console.error
override is injected in beforeStep(), which is too early because it gets wiped
out by page reloads during navigation steps. To fix this, move the injection
logic so that it runs in afterStep() instead, specifically re-inject the
console.error override after navigation steps complete inside the if
($this->shouldCaptureForStep(...)) block. This ensures the logger is active in
the new page context and captures errors on page load.
| Given I am on "/" | ||
| Then I should see no JavaScript errors |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Step wording not tracked by default filters
Because the context currently only listens for steps starting with “I visit” or “I go to”, the “Given I am on” navigation step will not trigger JS-error logging.
Either refactor the context (preferred – see previous comment) or change the step here to “Given I visit "/"”.
🏁 Script executed:
#!/bin/bash
# Search for step definitions in Behat contexts
rg -n "@(Given|When|Then)" -C2 --path-separator / .Length of output: 73022
🏁 Script executed:
#!/bin/bash
# Locate step definition for JS error logging in the Behat contexts
rg -n "JavaScript errors" -C2 --path-separator / tests/behat/bootstrapLength of output: 72
🏁 Script executed:
#!/bin/bash
# Search for the step definition of "Then I should see no JavaScript errors" across the repo.
rg -n "I should see no JavaScript errors" -C2 --path-separator /Length of output: 807
🏁 Script executed:
#!/bin/bash
# Display the beginning of src/JavascriptErrorContext.php to inspect navigation step filtering
sed -n '1,200p' src/JavascriptErrorContext.phpLength of output: 3429
Add “I am on” to JS‐error capture or update the feature step
By default JavascriptErrorContext::$stepFilters only matches I visit or I go to, so your “Given I am on "/"” step never instruments the page. You can either:
- Update the context’s filter to also match “I am on” (in src/JavascriptErrorContext.php):
private array $stepFilters = [ '/^I (visit|go to|am on)/', ];
- Or change your feature to use the existing filters (in tests/behat/features/js_error.feature):
Scenario: Visiting home page should not produce JS errors
- Given I am on "/"
- Given I visit "/"
Then I should see no JavaScript errors
<details>
<summary>🤖 Prompt for AI Agents</summary>
In tests/behat/features/js_error.feature around lines 5 to 6, the step "Given I
am on "/"" does not trigger JavaScript error logging because the context only
listens for steps starting with "I visit" or "I go to". To fix this, either
update the stepFilters array in src/JavascriptErrorContext.php to include "I am
on" by adding it to the regex pattern, or change the feature step here to "Given
I visit "/"" to match the existing filters.
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
#272
Summary by CodeRabbit