Feature/warn if dex is enabled in production#14
Merged
olajideolamide merged 13 commits intoMay 22, 2026
Conversation
Jump to issue if issue id is included in URL
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThis PR centralizes multi-database CI/test configuration, refactors request/response adapters to use type-aware helpers, introduces a test support layer (DexTestCase/DexDatabaseTestCase) with factories, and adds extensive database-backed unit, feature, lifecycle, repository, and journey tests. ChangesMonolithic PR — full review checkpoint
🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Config/Registrar.php (1)
12-12: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd strict typing declaration.
This file is missing the
declare(strict_types=1);declaration. As per coding guidelines, use strict typing where the file/project conventions allow it. Other source files in the codebase (e.g.,CiRequestPathProvider.php) consistently use strict typing.🔧 Proposed fix to add strict typing
<?php +declare(strict_types=1); + /** * This file is part of Dex.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Config/Registrar.php` at line 12, This file is missing a strict types declaration; add declare(strict_types=1); immediately after the opening <?php tag in the file containing namespace Dex\Config; (the Registrar file) so the Registrar class and any functions in the Dex\Config namespace are compiled with strict typing consistent with the project convention.src/Views/dex/_js.php (1)
219-243:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun deep-link open after row render, and only consume when matching issue is already open.
Current ordering/guard can skip proper row targeting and prematurely consume the deep-link state.
Suggested fix
function dexIssuesRenderRows() { const tbody = document.getElementById('issuesTbody'); const issues = dexIssuesState.data.issues || []; if (dexIssuesState.loading) { tbody.innerHTML = '<tr class="empty-row"><td colspan="8">Loading issues...</td></tr>'; return; } - - dexIssuesTryOpenDeepLinkedIssue(); if (!issues.length) { tbody.innerHTML = '<tr class="empty-row"><td colspan="8">No issues match your filters.</td></tr>'; + dexIssuesTryOpenDeepLinkedIssue(); return; } tbody.innerHTML = issues.map(dexIssuesBuildRow).join(''); + dexIssuesTryOpenDeepLinkedIssue(); } function dexIssuesTryOpenDeepLinkedIssue() { if (dexIssuesState.loading || dexIssuesDeepLinkState.opened || !dexIssuesDeepLinkState.issueId) { return; } - if (dexIssueDialogState.issueId) { + if (Number(dexIssueDialogState.issueId) === Number(dexIssuesDeepLinkState.issueId)) { dexIssuesDeepLinkState.opened = true; return; } const tbody = document.getElementById('issuesTbody'); const row = tbody?.querySelector(`tr[data-id="${dexIssuesDeepLinkState.issueId}"]`) || null; dexIssuesDeepLinkState.opened = true; dexIssuesOpenDialog(dexIssuesDeepLinkState.issueId, null, row); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Views/dex/_js.php` around lines 219 - 243, The deep-link logic currently runs/consumes before rows are rendered and prematurely sets dexIssuesDeepLinkState.opened; move the call to dexIssuesTryOpenDeepLinkedIssue() so it runs after tbody.innerHTML = issues.map(dexIssuesBuildRow).join(''), and change dexIssuesTryOpenDeepLinkedIssue() to only mark dexIssuesDeepLinkState.opened = true when the matching row/dialog is actually found or opened: check if dexIssueDialogState.issueId === dexIssuesDeepLinkState.issueId first (if so, set opened=true and return), otherwise locate the row element and only if row exists call dexIssuesOpenDialog(dexIssuesDeepLinkState.issueId, null, row) and then set opened=true; do not set opened=true unconditionally before confirming a match.
🧹 Nitpick comments (6)
tests/Support/Factories/RequestFactory.php (1)
61-61: 💤 Low valueSimplify boolean conversion.
The nested casting
(int) (((int) (...)) > 0)can be simplified to(int) (($summary['exception_count'] ?? 0) > 0)since the comparison already produces a boolean.♻️ Proposed simplification
- 'has_exception' => (int) (((int) ($summary['exception_count'] ?? 0)) > 0), + 'has_exception' => (int) (($summary['exception_count'] ?? 0) > 0),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Support/Factories/RequestFactory.php` at line 61, The expression used to set the 'has_exception' value in RequestFactory is overly nested: replace the nested casts around $summary['exception_count'] with a direct boolean comparison and single cast so use (int)(($summary['exception_count'] ?? 0) > 0) for the 'has_exception' array entry in the RequestFactory code to simplify and clarify intent.tests/Feature/IssueStatusActionFeatureTest.php (1)
82-101: ⚡ Quick winConsider extracting route setup to shared helper.
If multiple feature tests need identical Dex route configuration, extract
setupDexRoutes()to a shared trait or base class method to reduce duplication and ensure consistency across test suites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/IssueStatusActionFeatureTest.php` around lines 82 - 101, Extract the setupDexRoutes() helper into a shared test utility (either a base test class or trait) so other feature tests can reuse it: move the method that configures $filterConfig, sets aliases via self::FILTER_ALIAS -> DexUiFilter::class, injects mocks with Factories::injectMock('config','Filters',...) and Factories::injectMock('filters','filters',...), and registers the route group using self::PREFIX with the same route definitions and Services::injectMock('routes', $routes) into the new shared trait/base (e.g., DexRoutesTrait or TestCase::setupDexRoutes), update tests to call that shared method and remove duplicated copies to ensure consistent route setup across suites.tests/Support/Config/Registrar.php (1)
56-63: ⚡ Quick winDirectory creation side-effect in config method.
Creating the database directory during configuration violates the principle of config methods being pure. Consider moving this initialization to a test setup method (e.g., in
DexDatabaseTestCase::setUp()) or a dedicated bootstrap function.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Support/Config/Registrar.php` around lines 56 - 63, The Registrar config method currently creates the database directory (uses $dbPath and WRITEPATH and calls mkdir) which introduces a side-effect in configuration; remove the directory creation from the Registrar config implementation and instead add directory creation logic to test setup (e.g., implement in DexDatabaseTestCase::setUp() or a bootstrap helper) so tests ensure the directory exists before configuration runs; keep $dbPath/WRITEPATH usage in the config for the path value but move the mkdir/is_dir checks into the test setup or bootstrap function.tests/Feature/IssuesListFeatureTest.php (1)
87-116: ⚡ Quick winConsider extracting duplicate test setup helpers to base class.
The methods
setupDexRoutes()andallowLocalIp()are duplicated in bothIssuesListFeatureTestandIssueShowFeatureTest. Since both tests extendDexDatabaseTestCase, consider moving these helpers to the base class to reduce duplication and improve maintainability.♻️ Suggested refactor
Move
setupDexRoutes()andallowLocalIp()totests/Support/DexDatabaseTestCase.php:// In DexDatabaseTestCase.php protected function setupDexRoutes(string $filterAlias = 'dexui', string $prefix = 'dex'): void { $filterConfig = config('Filters'); $filterConfig->aliases[$filterAlias] = DexUiFilter::class; Factories::injectMock('config', 'Filters', $filterConfig); Factories::injectMock('filters', 'filters', $filterConfig); $routes = service('routes'); $routes->group($prefix, [ 'namespace' => 'Dex\\Controllers', 'filter' => $filterAlias, ], static function ($routes): void { $routes->get('', 'Issues::index'); $routes->get('issues/data', 'Issues::data'); $routes->get('issues/(:num)/dialog', 'Issues::dialog/$1'); $routes->post('issues/(:num)/resolve', 'Issues::resolve/$1'); $routes->post('issues/(:num)/ignore', 'Issues::ignore/$1'); }); Services::injectMock('routes', $routes); } protected function allowLocalIp(): void { $this->setDexEnv('DEX_ENABLED', '1'); $this->setDexEnv('DEX_UI_ENABLED', '1'); $this->setDexEnv('DEX_UI_ALLOWLIST', '127.0.0.1,::1'); $this->setDexEnv('DEX_UI_STEALTH_DENY', '0'); service('superglobals')->setServer('REMOTE_ADDR', '127.0.0.1'); }Then update the feature tests to call
parent::setupDexRoutes()andparent::allowLocalIp().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/IssuesListFeatureTest.php` around lines 87 - 116, The duplicated helper methods setupDexRoutes() and allowLocalIp() should be moved from IssuesListFeatureTest and IssueShowFeatureTest into the shared test base DexDatabaseTestCase to avoid duplication; create protected methods with the same names in DexDatabaseTestCase (keeping the existing logic that configures Filters, routes grouping and superglobals), remove the duplicates from the feature test classes, and update those tests to call the inherited setupDexRoutes() and allowLocalIp() (or parent::setupDexRoutes()/parent::allowLocalIp() if you prefer explicit calls).src/Adapters/CiHttpContextProvider.php (2)
88-92: ⚡ Quick winRemove unused foreach value binding to satisfy static analysis cleanly.
$_headeris never used; iterate over keys directly to avoid PHPMD noise.Proposed fix
- foreach ($this->requestHeaders($req) as $name => $_header) { + foreach (array_keys($this->requestHeaders($req)) as $name) { if ($maxHeaders > 0 && $i >= $maxHeaders) { break; } $line = (string) $req->getHeaderLine((string) $name);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Adapters/CiHttpContextProvider.php` around lines 88 - 92, The foreach binds an unused value ($_header) causing PHPMD noise; update the loop in CiHttpContextProvider to iterate keys only by replacing the current foreach ($this->requestHeaders($req) as $name => $_header) with an iteration over the keys (e.g., foreach (array_keys($this->requestHeaders($req)) as $name)) so the code still uses $req->getHeaderLine((string) $name) while removing the unused $_header binding.
108-136: ⚡ Quick winAdd PHPDoc for newly introduced helper methods.
requestHeaders()andresolveRequest()are new functions and should be documented per project standard.As per coding guidelines, "Add PHPDoc for new functions; comment only complex, non-obvious logic".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Adapters/CiHttpContextProvider.php` around lines 108 - 136, Add PHPDoc blocks for the two new methods: above requestHeaders(object $request) document a short description, annotate the parameter as `@param` object $request (note it accepts IncomingRequest|CLIRequest|any object with headers()/getHeaders()), and annotate `@return` array (empty array on failure) plus a brief line describing the header-extraction logic and fallback behavior; above resolveRequest() add a short description, annotate `@return` object|null (or the specific request type union if preferred) and mention that it uses service('request') and catches Throwable returning null. Keep the comments concise and only describe the non-obvious behavior (fallbacks and exception swallowing).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/phpunit.yml:
- Around line 53-60: Add an explicit least-privilege permissions block for the
GITHUB_TOKEN to the workflow and jobs; update the phpunit-mysql job (and the
other phpunit job referenced around lines 112-120) to include a permissions key
granting only what the CI needs (for example at minimum "contents: read" and any
other specific scopes like "packages: read" or "actions: read" if those jobs
push or download packages), or add a top-level permissions: { contents: read }
to the workflow if all jobs share the same minimal scope; ensure you add the
permissions block near the job definition for phpunit-mysql (job name/ID
"phpunit-mysql") and likewise for the other phpunit job so the default broad
token scopes are not used.
- Around line 89-95: The cache step "Cache Composer dependencies" uses
actions/cache@v4 for path: vendor and current key expression but runs on
pull_request and can write caches; modify the cache steps (the ones using
actions/cache@v4 with path: vendor and key: ${{ runner.os }}-php-${{ matrix.php
}}-composer-${{ hashFiles('**/composer.lock') }}) to only run for
non-pull_request events by adding an if condition (e.g. if: github.event_name !=
'pull_request') so PRs cannot prime or save the cache; apply the same change to
the other two actions/cache@v4 steps in the workflow.
- Around line 77-83: Update the workflow to pin reusable actions and harden
checkout credentials: replace mutable tags like actions/checkout@v4,
shivammathur/setup-php@v2 and actions/cache@v4 with immutable references (SHA or
full ref) for each occurrence and add persist-credentials: false to the
actions/checkout step; also add a minimal permissions: block at the workflow or
job level (e.g., read-only permissions for contents and other needed scopes) to
limit token scope. Apply these same changes to the corresponding steps in the
phpunit, phpunit-sqlite, phpunit-mysql, and phpunit-mariadb jobs (the
actions/checkout, Setup PHP, and actions/cache steps).
In `@composer.json`:
- Around line 46-47: PHPStan 2.0 fails because the configured CodeIgniter
PHPStan extension file vendor/codeigniter/phpstan-codeigniter/extension.neon is
missing/incompatible; either upgrade or remove the package reference in
composer.json (codeigniter/phpstan-codeigniter) to a PHPStan-2-compatible
release, or stop including its extension in your config; specifically update
composer.json entries for "codeigniter/phpstan-codeigniter" / "phpstan/phpstan"
and modify your phpstan.neon/phpstan.neon.dist to remove or guard the include of
vendor/codeigniter/phpstan-codeigniter/extension.neon (use a
conditional/existence check or delete the include) so the `composer phpstan` run
no longer fails.
In `@src/Adapters/CiHttpContextProvider.php`:
- Around line 53-58: In CiHttpContextProvider's request/context extraction
blocks (the try { $out['url'] = (string) $uri; $out['query'] = (string)
$uri->getQuery(); } catch (Throwable) { /* ignore */ }) and the similar catch
blocks at the other locations, do not swallow exceptions silently; instead catch
Throwable $e and call the class logger (e.g., $this->logger->error) with a clear
message and contextual data (include $uri, any request identifiers, and the
partial $out array), then either set the safe fallbacks
($out['url']=null/$out['query']='') or rethrow the exception depending on caller
expectations; update all occurrences (the blocks around $uri usage at the shown
spots) to follow this pattern so exceptions are logged with context.
In `@src/Adapters/CiRouterInfoProvider.php`:
- Around line 64-66: The catch blocks in CiRouterInfoProvider that currently
read "catch (Throwable) { // ignore }" must not silently swallow exceptions;
replace each empty catch with logging the Throwable and contextual data before
continuing the fallback path. In the CiRouterInfoProvider class (the methods
where you catch Throwable), call the existing logger (e.g.
$this->logger->warning or ->error) and include the exception message/stack and
relevant identifiers (route name, CI identifier, method like
getRouterInfo/resolveRouterInfo) so the log provides context, then continue with
the fallback behavior as before.
In `@src/Config/Registrar.php`:
- Line 25: Registrar currently only registers the 'dexui' filter alias
(DexUiFilter::class) while docs still reference 'dex-ui'; either restore
backwards-compatibility by adding a 'dex-ui' => DexUiFilter::class entry
alongside 'dexui' in the Registrar (and add a comment indicating it is
deprecated) or update the documentation to use the new 'dexui' alias and add a
clear breaking-change note; update the file referenced in the comment
(docs/content/security/protecting-the-dashboard.md) to match whichever approach
you choose so routes/tests and docs stay consistent.
In `@src/Support/RequestSnapshot.php`:
- Around line 54-62: The try/catch around extracting $uri, $url, $query, $host,
$scheme in RequestSnapshot (the block accessing $req->getUri()) currently
swallows Throwable; change it to catch (Throwable $e) and log the exception with
context (include $req or the partial values like $url/$host if available)
instead of ignoring it—use the class's logger or a standard logger to emit the
error and any useful context so failures building the URI snapshot are recorded
for production diagnosis.
In `@tests/Adapters/CiCacheStoreTest.php`:
- Around line 13-18: The teardown currently resets the cache singleton before
invoking the parent's teardown which may rely on the cache; change the order in
the tearDown() method so parent::tearDown() is called first and then
Services::resetSingle('cache') is called afterwards to avoid breaking parent
cleanup logic.
In `@tests/Journeys/RepeatedIssueJourneyTest.php`:
- Line 70: In RepeatedIssueJourneyTest (the test that sets last_seen to
'2025-01-02 10:00:00'), remove the ineffective sleep(0) call—it's a no-op in PHP
and unnecessary because last_seen is explicitly set; simply delete the line
containing sleep(0) so the test relies only on the explicit last_seen assignment
in the test method.
In `@tests/Lifecycle/LifecycleJsonTest.php`:
- Around line 150-153: The test currently passes a valid JSON object string
('{}') to RequestFactory::normal, so update the fixture to use a truly
non-object/malformed lifecycle payload (for example a JSON string or primitive
like '"invalid"', 'null', or '42') when setting 'lifecycle_json' so the
non-object path is exercised; change the call to RequestFactory::normal that
sets 'lifecycle_json' (the $row setup in LifecycleJsonTest) to one of these
non-object values and keep the rest of the test assertions unchanged.
In `@tests/Lifecycle/LifecycleStorageTest.php`:
- Around line 86-91: The test currently filters span items into $spans and then
asserts durations, but it can pass vacuously if $spans is empty; update the test
(the array_filter result $spans in LifecycleStorageTest) to first assert that
$spans is not empty (e.g. use $this->assertNotEmpty($spans) or
$this->assertGreaterThan(0, count($spans))) before iterating, then keep the
existing checks on each $span['duration_ms'] to ensure the test fails when no
spans exist and still validates durations when they do.
In `@tests/Support/Config/Registrar.php`:
- Line 108: The current assignment for the 'port' config in Registrar.php casts
getenv('DB_PORT') to int without validation; update the logic that sets the
'port' array entry (in the Registrar class/config builder) to first check that
getenv('DB_PORT') is a valid numeric integer (e.g., is_numeric or filter_var
with FILTER_VALIDATE_INT) and only cast when valid, otherwise fall back to the
default 3306 (or throw a clear configuration error); ensure you reference the
same 'port' => ... expression so you don't change other config keys.
In `@tests/Support/Factories/IssueFactory.php`:
- Around line 54-57: The regressed() factory returns status 'regression' which
mismatches the method name; update the regressed(array $overrides = []) method
to return array_merge(self::base(), ['status' => 'regressed'], $overrides) so
the status value matches the regressed() helper (refer to regressed() and
base()).
In `@tests/Telemetry/RequestLifecycleServiceTest.php`:
- Line 46: The test in RequestLifecycleServiceTest weakened the assertion from a
bounded timing check to just assertIsNumeric($span['end_ms']), which can mask
duration regressions; replace that assertion with a tolerant upper-bound check
(e.g., $this->assertLessThanOrEqual(160, $span['end_ms'])) to restore validation
that end_ms does not exceed the expected request duration while allowing small
timing variance; update the assertion in the test method that inspects
$span['end_ms'] (the line currently using assertIsNumeric($span['end_ms'])) to
use assertLessThanOrEqual with the suggested tolerance.
---
Outside diff comments:
In `@src/Config/Registrar.php`:
- Line 12: This file is missing a strict types declaration; add
declare(strict_types=1); immediately after the opening <?php tag in the file
containing namespace Dex\Config; (the Registrar file) so the Registrar class and
any functions in the Dex\Config namespace are compiled with strict typing
consistent with the project convention.
In `@src/Views/dex/_js.php`:
- Around line 219-243: The deep-link logic currently runs/consumes before rows
are rendered and prematurely sets dexIssuesDeepLinkState.opened; move the call
to dexIssuesTryOpenDeepLinkedIssue() so it runs after tbody.innerHTML =
issues.map(dexIssuesBuildRow).join(''), and change
dexIssuesTryOpenDeepLinkedIssue() to only mark dexIssuesDeepLinkState.opened =
true when the matching row/dialog is actually found or opened: check if
dexIssueDialogState.issueId === dexIssuesDeepLinkState.issueId first (if so, set
opened=true and return), otherwise locate the row element and only if row exists
call dexIssuesOpenDialog(dexIssuesDeepLinkState.issueId, null, row) and then set
opened=true; do not set opened=true unconditionally before confirming a match.
---
Nitpick comments:
In `@src/Adapters/CiHttpContextProvider.php`:
- Around line 88-92: The foreach binds an unused value ($_header) causing PHPMD
noise; update the loop in CiHttpContextProvider to iterate keys only by
replacing the current foreach ($this->requestHeaders($req) as $name => $_header)
with an iteration over the keys (e.g., foreach
(array_keys($this->requestHeaders($req)) as $name)) so the code still uses
$req->getHeaderLine((string) $name) while removing the unused $_header binding.
- Around line 108-136: Add PHPDoc blocks for the two new methods: above
requestHeaders(object $request) document a short description, annotate the
parameter as `@param` object $request (note it accepts
IncomingRequest|CLIRequest|any object with headers()/getHeaders()), and annotate
`@return` array (empty array on failure) plus a brief line describing the
header-extraction logic and fallback behavior; above resolveRequest() add a
short description, annotate `@return` object|null (or the specific request type
union if preferred) and mention that it uses service('request') and catches
Throwable returning null. Keep the comments concise and only describe the
non-obvious behavior (fallbacks and exception swallowing).
In `@tests/Feature/IssuesListFeatureTest.php`:
- Around line 87-116: The duplicated helper methods setupDexRoutes() and
allowLocalIp() should be moved from IssuesListFeatureTest and
IssueShowFeatureTest into the shared test base DexDatabaseTestCase to avoid
duplication; create protected methods with the same names in DexDatabaseTestCase
(keeping the existing logic that configures Filters, routes grouping and
superglobals), remove the duplicates from the feature test classes, and update
those tests to call the inherited setupDexRoutes() and allowLocalIp() (or
parent::setupDexRoutes()/parent::allowLocalIp() if you prefer explicit calls).
In `@tests/Feature/IssueStatusActionFeatureTest.php`:
- Around line 82-101: Extract the setupDexRoutes() helper into a shared test
utility (either a base test class or trait) so other feature tests can reuse it:
move the method that configures $filterConfig, sets aliases via
self::FILTER_ALIAS -> DexUiFilter::class, injects mocks with
Factories::injectMock('config','Filters',...) and
Factories::injectMock('filters','filters',...), and registers the route group
using self::PREFIX with the same route definitions and
Services::injectMock('routes', $routes) into the new shared trait/base (e.g.,
DexRoutesTrait or TestCase::setupDexRoutes), update tests to call that shared
method and remove duplicated copies to ensure consistent route setup across
suites.
In `@tests/Support/Config/Registrar.php`:
- Around line 56-63: The Registrar config method currently creates the database
directory (uses $dbPath and WRITEPATH and calls mkdir) which introduces a
side-effect in configuration; remove the directory creation from the Registrar
config implementation and instead add directory creation logic to test setup
(e.g., implement in DexDatabaseTestCase::setUp() or a bootstrap helper) so tests
ensure the directory exists before configuration runs; keep $dbPath/WRITEPATH
usage in the config for the path value but move the mkdir/is_dir checks into the
test setup or bootstrap function.
In `@tests/Support/Factories/RequestFactory.php`:
- Line 61: The expression used to set the 'has_exception' value in
RequestFactory is overly nested: replace the nested casts around
$summary['exception_count'] with a direct boolean comparison and single cast so
use (int)(($summary['exception_count'] ?? 0) > 0) for the 'has_exception' array
entry in the RequestFactory code to simplify and clarify intent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91f314b6-7b50-4844-9bd2-2ed035c27f57
📒 Files selected for processing (61)
.github/workflows/phpunit.yml.gitignoreREADME.mdcomposer.jsonphpstan-baseline.neonphpstan-baseline.neon.cleanedphpstan-baseline.neon.newphpstan.neon.distphpunit.xml.distsrc/Adapters/CiHttpContextProvider.phpsrc/Adapters/CiRequestMetaFactory.phpsrc/Adapters/CiRequestPathProvider.phpsrc/Adapters/CiResponseApplier.phpsrc/Adapters/CiRouterInfoProvider.phpsrc/Config/Registrar.phpsrc/Config/Routes.phpsrc/Controllers/Issues.phpsrc/Dex.phpsrc/Filters/DexUiFilter.phpsrc/Repositories/IssueRepository.phpsrc/Support/ConfigResolver.phpsrc/Support/RequestSnapshot.phpsrc/Views/dex/_js.phpsrc/Views/dex/issues_dialog_shell.phpsrc/Views/dex/issues_list.phpsrc/Views/dex/layout.phptests/Adapters/CiCacheStoreTest.phptests/Adapters/CiHttpContextProviderTest.phptests/Adapters/CiRequestPathProviderTest.phptests/Adapters/CiRouterInfoProviderTest.phptests/Database/MigrationTest.phptests/Feature/DashboardAccessTest.phptests/Feature/IssueShowFeatureTest.phptests/Feature/IssueStatusActionFeatureTest.phptests/Feature/IssuesListFeatureTest.phptests/Filters/DexUiFilterFeatureTest.phptests/Filters/DexUiFilterTest.phptests/Journeys/IgnoredIssueJourneyTest.phptests/Journeys/NewIssueJourneyTest.phptests/Journeys/RepeatedIssueJourneyTest.phptests/Journeys/ResolvedRegressionJourneyTest.phptests/Lifecycle/LifecycleJsonTest.phptests/Lifecycle/LifecycleMetricsTest.phptests/Lifecycle/LifecycleStorageTest.phptests/Repositories/IssueReadRepositoryTest.phptests/Repositories/IssueRepositoryTest.phptests/Repositories/OccurrenceReadRepositoryTest.phptests/Repositories/OccurrenceRepositoryTest.phptests/Repositories/RequestReadRepositoryTest.phptests/Repositories/RequestRepositoryTest.phptests/Support/CachedConfigProviderTest.phptests/Support/Config/Registrar.phptests/Support/ConfigResolverTest.phptests/Support/DexDatabaseTestCase.phptests/Support/DexTestCase.phptests/Support/Factories/IssueFactory.phptests/Support/Factories/LifecycleFactory.phptests/Support/Factories/OccurrenceFactory.phptests/Support/Factories/RequestFactory.phptests/Telemetry/RequestLifecycleServiceTest.phptests/bootstrap.php
💤 Files with no reviewable changes (3)
- src/Views/dex/issues_dialog_shell.php
- src/Adapters/CiResponseApplier.php
- src/Views/dex/layout.php
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/phpunit.yml:
- Around line 9-11: The workflow-wide permissions currently grant actions: write
for all runs; narrow them by changing the top-level permissions block
(permissions:) to contents: read and actions: read, then remove or downgrade any
job-level actions: write in the phpunit-sqlite, phpunit-mysql, and
phpunit-mariadb jobs; re-grant actions: write only where cache saving occurs
(the step using actions/cache@v4) and only for non-PR runs (e.g., add an if:
github.event_name != 'pull_request' on that save step or on the job that saves
cache) so GITHUB_TOKEN write scope is limited to the cache-save path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dbbb2622-0e2f-4ed7-865e-76d8c0bc6563
📒 Files selected for processing (1)
.github/workflows/phpunit.yml
Owner
Author
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
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
2 participants
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.
Summary
Warn if dex is in production
Jump to issue if issue id is included in URL