feat: Add matrix/table comparison from csv data#185
feat: Add matrix/table comparison from csv data#185Json-To-String wants to merge 27 commits intoTheBSD:developmentfrom
Conversation
|
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 ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a CSV-driven "matrix alternatives" feature: a new controller with index/show/details endpoints that parse CSVs (including transposed format), three Blade views (index, show, details), a header navigation link, and three routes exposing the functionality. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@app/Http/Controllers/MatrixAlternativesController.php`:
- Around line 67-71: The code reads CSV headers with $headers = fgetcsv($handle)
but doesn't check for false before using count() and array_combine(), which can
throw warnings; update the import logic in MatrixAlternativesController (the
block using $headers, $row, fgetcsv and array_combine) to first verify $headers
!== false && is_array($headers) and handle the error/empty-file case (e.g.,
log/return early or skip processing), and inside the loop ensure you only call
count($headers) and array_combine($headers, $row) when $headers is a valid
array.
In `@resources/views/matrix/show.blade.php`:
- Line 205: Replace the redundant null-coalescing expression
$selected['developerExperience'] ?? ($selected['developerExperience'] ?? '—')
with a single fallback expression like $selected['developerExperience'] ?? '—'
in the Blade view so the Developer Experience paragraph uses the key
$selected['developerExperience'] with a single default '—'.
- Around line 124-126: The template can render "null%" when
$selected['totalScore'] is missing because $toPercent($selected['totalScore'])
returns null and the null-coalescing only applies to the whole expression with
the "%" appended; change the output to compute a safe fallback first (e.g.,
$percent = $toPercent($selected['totalScore'] ?? null) ?? 'N/A' or '0') and then
render "{{ $percent }}%" or "{{ $percent }}" accordingly so the % is not shown
for non-numeric fallbacks; update the line that prints Total Score to use
$toPercent and $selected['totalScore'] together with an explicit fallback
instead of relying on the current null coalescing.
🧹 Nitpick comments (10)
app/Http/Controllers/MatrixAlternativesController.php (3)
10-11: Add explicit return types and use PHPDoc instead of inline comments.The
index()method is missing an explicit return type declaration. Per coding guidelines, always declare explicit return types. Also prefer PHPDoc blocks over inline comments.Proposed fix
- // List available company CSVs in storage/app/matrix - public function index() + /** + * List available company CSVs in storage/app/matrix. + */ + public function index(): \Illuminate\Contracts\View\View|\Illuminate\Http\RedirectResponseAs per coding guidelines: "Always declare explicit return types for functions/methods" and "Prefer PHPDoc blocks over inline code comments."
45-46: Add explicit return type and type hint for$alternativeparameter.The
show()method is missing a return type declaration and the$alternativeparameter lacks a type hint.Proposed fix
- // Show matrix for a company and optional alternative detail - public function show(string $company, $alternative = null) + /** + * Show matrix for a company and optional alternative detail. + */ + public function show(string $company, ?string $alternative = null): \Illuminate\Contracts\View\ViewAs per coding guidelines: "Always declare explicit return types" and "Use appropriate PHP type hints for method parameters."
66-74: Potential file handle leak if CSV parsing fails unexpectedly.If an exception occurs during CSV parsing, the file handle may not be closed. Consider using a try/finally block or Laravel's file helpers.
Safer approach using try/finally
$rows = []; - if (($handle = fopen($file, 'r')) !== false) { - $headers = fgetcsv($handle); - while (($row = fgetcsv($handle)) !== false) { - if (count($row) === count($headers)) { - $rows[] = array_combine($headers, $row); + $handle = fopen($file, 'r'); + if ($handle !== false) { + try { + $headers = fgetcsv($handle); + while (($row = fgetcsv($handle)) !== false) { + if (count($row) === count($headers)) { + $rows[] = array_combine($headers, $row); + } } + } finally { + fclose($handle); } - fclose($handle); }resources/views/partials/header.blade.php (1)
215-236: Mobile nav item follows existing patterns.The implementation is consistent with other mobile navigation items. Consider using a more distinctive icon (e.g., a grid/table icon) instead of the hamburger menu lines to better represent "Matrix" comparisons, but this is a minor UI polish item.
resources/views/matrix/index.blade.php (2)
22-34: Consider using slug instead of name in route parameters for cleaner URLs.Passing
$c['name']directly may produce URLs with spaces or special characters. Using the slug would produce cleaner, more predictable URLs.Proposed fix
- <a href="{{ route('matrix.show', ['company' => $c['name']]) }}" class="flex flex-col items-start gap-3 p-4 border border-gray-100 rounded-lg hover:shadow-md transition-shadow bg-white"> + <a href="{{ route('matrix.show', ['company' => $c['slug']]) }}" class="flex flex-col items-start gap-3 p-4 border border-gray-100 rounded-lg hover:shadow-md transition-shadow bg-white dark:bg-gray-800 dark:border-gray-700">
1-42: Consider adding dark mode support.The view uses hardcoded light-mode colors (e.g.,
bg-white,text-gray-500,border-gray-100) withoutdark:variants. Per coding guidelines, support dark mode consistently usingdark:variants where the project does.resources/views/matrix/show.blade.php (4)
10-23: Consider extracting best-score computation to the controller.This logic block computes
$bestScoreby iterating rows. Moving this to the controller would keep the view cleaner and more focused on presentation.
13-13: Use an alias or import forStrinstead of fully-qualified class name.Multiple occurrences of
\Illuminate\Support\Str::slug()in the view. Consider adding@php use Illuminate\Support\Str;@endphp`` at the top or passing pre-slugified values from the controller.Add import at top of file
`@php` use Illuminate\Support\Str; `@endphp` <x-app-layout> ...Then use
Str::slug()directly.Also applies to: 44-44, 87-87, 90-90
48-48: Potential Tailwind class conflict withodd:/even:and conditionalbg-accent/10.When
$isBestis true, bothodd:bg-white even:bg-gray-50andbg-accent/10are applied. The conditional background may not consistently override the zebra striping depending on specificity. Consider using a ternary to apply mutually exclusive classes.Proposed fix
- <tr class="odd:bg-white even:bg-gray-50 {{ $isBest ? 'bg-accent/10 border-l-4 border-l-accent' : '' }}"> + <tr class="{{ $isBest ? 'bg-accent/10 border-l-4 border-l-accent' : 'odd:bg-white even:bg-gray-50' }}">
1-252: Consider adding dark mode support.The view lacks
dark:variant classes for backgrounds, borders, and text colors. Per coding guidelines, support dark mode consistently where the project does.
Json-To-String
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@app/Http/Controllers/MatrixAlternativesController.php`:
- Around line 10-12: Update MatrixAlternativesController: add explicit return
type declarations to the index() and show() methods (e.g., add :
\Illuminate\View\View or another appropriate Response type used in your app) and
type-hint the $alternative parameter/variable with the Alternative model (or the
correct model/class name) in show(); move the inline method notes/comments into
proper PHPDoc blocks above each method describing the contract and return type
(use `@param/`@return annotations) so index(), show(), and $alternative are
explicitly typed.
- Around line 48-63: The code is using the raw $company value to build a
filesystem path; instead, validate $company via a Form Request (e.g., require
pattern/allowed values) and replace direct concatenation with a whitelist
lookup: scan storage_path('app/matrix') once to build a map of
Str::slug(pathinfo($file, PATHINFO_FILENAME)) => absolute path, then resolve the
requested file by slug (use Str::slug($request->company) or a validated slug
field) to fetch the safe path (do not use $company in storage_path
concatenation); if no match, abort 404. Update MatrixAlternativesController to
use the Form Request and the slug->path map (and remove direct $file = $dir .
'/' . $company . '.csv' usage).
In `@resources/views/matrix/show.blade.php`:
- Around line 60-62: The TOP badge uses an invalid Tailwind class; update the
span inside the `@if` ($isBest && !$isSearched) block (the <span class="..."> for
the TOP badge) to replace the incorrect text-white-700 class with the correct
text-white class so the badge renders with white text under Tailwind v3.
- Around line 104-107: Default $searched to an empty array instead of null and
guard array accesses: change $searched = $searchedCompany ?? [] and use
isset($searched['logo']) when computing $searchedLogo; compute percent values
before rendering by calling $toPercent($selected['totalScore'] ?? 0) and
similarly for $searched using null coalescing so the template never calls
toPercent with null; validate website URLs before rendering by using a
server-side check (e.g., filter_var($url, FILTER_VALIDATE_URL) and
parse_url($url, PHP_URL_SCHEME) in ['http','https']) and only render anchor tags
when the scheme is http/https (or sanitize/skip otherwise); apply these checks
around uses of $selected, $searched, $selected['totalScore'], and the website
link output to avoid unsafe or missing data.
🧹 Nitpick comments (2)
app/Http/Controllers/MatrixAlternativesController.php (1)
22-30: Consider preloading companies to avoid one query per CSV.This loop issues a query per CSV file. If the directory grows, that becomes an N+1-style pattern. Consider preloading all matching companies once (by slug/name) and mapping in memory.
♻️ Possible approach
- foreach (glob($dir . '/*.csv') as $path) { + $paths = glob($dir . '/*.csv'); + $names = array_map(fn ($path) => pathinfo($path, PATHINFO_FILENAME), $paths); + $slugs = array_map(fn ($name) => Str::slug($name), $names); + $companies = Company::query() + ->whereIn('slug', $slugs) + ->orWhereIn('name', $names) + ->get(['slug', 'name', 'image_path']) + ->keyBy('slug'); + $companiesByName = $companies->keyBy('name'); + foreach ($paths as $path) {- $companyModel = Company::query()->where('slug', $slug) - ->orWhere('name', $name) - ->first(); + $companyModel = $companies[$slug] ?? $companiesByName[$name] ?? null;resources/views/matrix/show.blade.php (1)
108-112: Use curly braces for PHP control structures inside@php.Single-line
ifstatements in the closures should use braces for consistency and to meet the project’s PHP style rules.✅ Proposed update
- $toPercent = function($score) { - if ($score === null) return null; - $s = (int)$score; - return round(($s / 25) * 100); - }; + $toPercent = function($score) { + if ($score === null) { + return null; + } + $s = (int)$score; + return round(($s / 25) * 100); + };- $renderFeature = function($row, $key) { - if (!$row) return '—'; - if (isset($row[$key]) && $row[$key] !== '') return $row[$key]; - return '—'; - }; + $renderFeature = function($row, $key) { + if (!$row) { + return '—'; + } + if (isset($row[$key]) && $row[$key] !== '') { + return $row[$key]; + } + return '—'; + };As per coding guidelines, always use curly braces for control structures, even for single-line bodies.
Also applies to: 182-185
| // List available company CSVs in storage/app/matrix | ||
| public function index() | ||
| { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n app/Http/Controllers/MatrixAlternativesController.phpRepository: TheBSD/poykott
Length of output: 4119
Add explicit return types and move method notes to PHPDoc.
index() and show() are missing return types, and $alternative lacks a type hint. Replace inline comments with PHPDoc blocks to clarify method contracts.
Proposed update
- // List available company CSVs in storage/app/matrix
- public function index()
+ /**
+ * List available company CSVs in storage/app/matrix.
+ */
+ public function index(): \Illuminate\Http\RedirectResponse|\Illuminate\View\View- // Show matrix for a company and optional alternative detail
- public function show(string $company, $alternative = null)
+ /**
+ * Show matrix for a company and optional alternative detail.
+ */
+ public function show(string $company, ?string $alternative = null): \Illuminate\View\View🤖 Prompt for AI Agents
In `@app/Http/Controllers/MatrixAlternativesController.php` around lines 10 - 12,
Update MatrixAlternativesController: add explicit return type declarations to
the index() and show() methods (e.g., add : \Illuminate\View\View or another
appropriate Response type used in your app) and type-hint the $alternative
parameter/variable with the Alternative model (or the correct model/class name)
in show(); move the inline method notes/comments into proper PHPDoc blocks above
each method describing the contract and return type (use `@param/`@return
annotations) so index(), show(), and $alternative are explicitly typed.
| @if ($isBest && !$isSearched) | ||
| <span class="text-xs bg-green-400 border border-green-400 px-2 py-0.5 rounded text-white-700">TOP</span> | ||
| @endif |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Tailwind CSS v3 text color classes text-white
💡 Result:
In Tailwind CSS v3, text-white is a text color utility that sets an element’s text color to white (part of the “Text Color” utilities quick reference). [1]
Example:
<p class="text-white">White text</p>You can combine it with variants like dark: (example shown in the v3 dark mode docs where an icon uses text-white). [2]
Sources: [1] Tailwind CSS v3 docs (Text Color quick reference), [2] Tailwind CSS v3 docs (Dark Mode example)
🌐 Web query:
Tailwind CSS v3 text-white-700 class does it exist
💡 Result:
No — text-white-700 does not exist in Tailwind CSS v3. The default palette defines white as a single color (text-white), not a shaded scale with 50–900 variants. Tailwind’s v3 text-color reference lists text-white but no text-white-* classes. [1]
If you want “darker/lighter white”, use:
- a light gray like
text-slate-100/text-gray-100, etc. [1] - or an arbitrary value like
text-[#f8fafc] - or define a custom
whitescale in your Tailwind config.
Sources: [1]
Fix invalid Tailwind v3 class on the TOP badge.
text-white-700 is not a valid Tailwind v3 class; use text-white instead.
Proposed fix
- <span class="text-xs bg-green-400 border border-green-400 px-2 py-0.5 rounded text-white-700">TOP</span>
+ <span class="text-xs bg-green-400 border border-green-400 px-2 py-0.5 rounded text-white">TOP</span>📝 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.
| @if ($isBest && !$isSearched) | |
| <span class="text-xs bg-green-400 border border-green-400 px-2 py-0.5 rounded text-white-700">TOP</span> | |
| @endif | |
| `@if` ($isBest && !$isSearched) | |
| <span class="text-xs bg-green-400 border border-green-400 px-2 py-0.5 rounded text-white">TOP</span> | |
| `@endif` |
🤖 Prompt for AI Agents
In `@resources/views/matrix/show.blade.php` around lines 60 - 62, The TOP badge
uses an invalid Tailwind class; update the span inside the `@if` ($isBest &&
!$isSearched) block (the <span class="..."> for the TOP badge) to replace the
incorrect text-white-700 class with the correct text-white class so the badge
renders with white text under Tailwind v3.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@app/Http/Controllers/MatrixAlternativesController.php`:
- Around line 105-163: The details() method duplicates CSV loading and row
lookup logic from show(); extract that logic into a private method (e.g.
loadMatrixData(string $company, ?string $alternative = null): array) that
performs slug-based file discovery, opens/parses the CSV into $rows, and
computes $selected and $searchedCompany, then have both show() and details()
call loadMatrixData(...) and pass the returned
['rows','selected','searchedCompany'] to their respective views (matrix.show and
matrix.details) instead of duplicating the parsing code; update function
signatures to return the view with ['company' => $company] + $data.
In `@resources/views/matrix/details.blade.php`:
- Line 41: The Blade template unconditionally appends "%" after the computed
value, causing "—%" when $searched['totalScore'] is missing; update the
expression used in the Total Score line so the percent sign is included only
when a real percentage is rendered (use the same pattern as line 31): evaluate
$toPercent($searched['totalScore'] ?? null) ?? ($searched['totalScore'] ?? '—')
and append "%" only when that result is not the fallback '—' (i.e., render
either "{value}%" or "—"); locate the expression in the <p> with "Total Score"
and modify it so the "%" is inside the conditional output rather than outside.
- Around line 147-148: The blade uses $searched[$key] directly which will throw
if $searched is null; update the cell rendering logic where $searched and $key
are used to first check that $searched is not null before indexing (e.g., use a
conditional that returns '—' when $searched is null, otherwise return
$searched[$key] if set, or '—' as fallback), or precompute a safe value for the
template (e.g., assign a safe $searched array or value before the table) so that
the expression referencing $searched[$key] cannot run on a null value.
- Around line 58-61: The template accesses $searched['name'],
$searched['presence'] and $searched['description'] without guarding against
$searched being null; update the Blade snippet that renders these values (the
lines referencing $searched) to first check $searched (or use the optional()
helper or data_get) before indexing into it so you don't access array keys on
null, and ensure the presence/description fallback logic uses a null-safe
expression that returns 'No presence data available.' when $searched is null or
neither key exists.
🧹 Nitpick comments (2)
resources/views/matrix/details.blade.php (1)
5-5: Consider using semantic color classes for dark mode consistency.
text-gray-600doesn't automatically adapt to dark mode. If the project uses semantic classes liketext-muted-foreground(seen elsewhere in this file), consider using those for consistency.Proposed fix
- <p class="text-sm text-gray-600">Detailed comparison between the selected alternative and the searched company.</p> + <p class="text-sm text-muted-foreground">Detailed comparison between the selected alternative and the searched company.</p>Same applies to line 158:
- <p class="text-gray-500">No alternative selected or not found.</p> + <p class="text-muted-foreground">No alternative selected or not found.</p>app/Http/Controllers/MatrixAlternativesController.php (1)
22-40: N+1 query: Company lookup inside loop.Each CSV file triggers a separate database query to find the matching Company model. With many CSV files, this becomes inefficient.
Proposed fix: Pre-fetch all companies
if (is_dir($dir)) { + // Pre-fetch relevant companies to avoid N+1 + $csvNames = collect(glob($dir . '/*.csv')) + ->map(fn ($path) => pathinfo($path, PATHINFO_FILENAME)); + $slugs = $csvNames->map(fn ($name) => Str::slug($name)); + + $companies = Company::query() + ->whereIn('slug', $slugs) + ->orWhereIn('name', $csvNames) + ->get() + ->keyBy(fn ($c) => Str::slug($c->slug ?: $c->name)); + foreach (glob($dir . '/*.csv') as $path) { $name = pathinfo($path, PATHINFO_FILENAME); $slug = Str::slug($name); - // Try to find a Company model so we can use the media library image_path. - $companyModel = Company::query()->where('slug', $slug) - ->orWhere('name', $name) - ->first(); + $companyModel = $companies[$slug] ?? null; $imagePath = $companyModel?->image_path ?? asset('images/logos/' . $slug . '.svg');
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@app/Http/Controllers/MatrixAlternativesController.php`:
- Around line 634-639: The current bidirectional partial-match using
str_contains($normKey, $normalizedKey) || str_contains($normalizedKey, $normKey)
can return incorrect/ambiguous values (e.g., "tier" matching "freetier"); change
the fallback to a stricter check: first require an exact match, then allow only
a single-directional starts-with match (e.g., str_starts_with($normKey,
$normalizedKey) or the inverse depending on intent) when iterating
$normalizedMap with $normKey/$normalizedKey/$value so "tier" does not match
"freetier"; apply the same exact->starts-with change in getCellScore() where the
identical bidirectional check occurs.
- Around line 22-40: The loop in MatrixAlternativesController reads CSV files
and runs Company::query()->where('slug', $slug)->orWhere('name', $name)->first()
per file causing N+1 DB queries; instead, before iterating over files gather all
slugs and names (from pathinfo/Str::slug), run one batched query on the Company
model to fetch all matching companies (e.g., whereIn('slug', $slugs) and
whereIn('name', $names)), build an in-memory map keyed by slug and/or name, and
then inside the foreach use that map to set $companyModel/$imagePath (falling
back to asset(...)) and populate $files — update references to $companyModel,
$slug, $name accordingly.
- Around line 491-493: The code currently sets $totalScore to only
$mappedRows[$i]['islPresence'], but it should sum all four category scores like
the fallback branch does; update the assignment at the place where $totalScore
and $mappedRows[$i]['totalScore'] are set so $totalScore is the sum of the four
category score fields (use the same four keys used in the fallback branch), or
replicate the fallback branch accumulation logic and then assign
$mappedRows[$i]['totalScore'] = $totalScore; ensure you reference the same array
keys on $mappedRows[$i] rather than only 'islPresence'.
- Around line 573-584: The formatScoreAsPercent(?int $score) implementation is
effectively an identity ( ($score/4)/25*100 == $score ), so either remove the
function and callers if scores are already 0–100, or fix the math to actually
map the intended source range to percent; for example, to map a 0–4 scale to
0–100 use round(($score / 4) * 100) (preserve the null handling), or if the
input is 0–25 and you intended 0–100 use round(($score / 25) * 100); update the
function body accordingly and adjust any callers expecting an int percent.
🧹 Nitpick comments (3)
app/Http/Controllers/MatrixAlternativesController.php (3)
429-448:$toNumberclosure and$metricKeyMapare defined identically in both branches ofparseMatrixCsv.Extract these to a private method (for
$toNumber) and a shared variable or class constant (for$metricKeyMap) before theif ($isTransposed)branch to eliminate duplication.Also applies to: 518-535
172-497:parseMatrixCsv()is ~325 lines with deeply nested responsibilities.This method handles CSV reading, format detection, transposed-format parsing, metric normalization, hierarchy building, and score computation all in one. Consider decomposing into focused private methods, e.g.:
detectCsvFormat(array $raw): boolparseTransposedCsv(array $raw): arraynormalizeMetricKeys(array $rows): arraycomputeCategoryScores(array $rows): arrayThis would improve testability and readability significantly.
91-93: Consider moving cell-rendering logic out of closures passed to views.Passing closures to Blade templates works but is unconventional — they can't be serialized (breaks view caching) and are harder to test. Consider using a dedicated Blade component or a simple helper/presenter class instead.
Also applies to: 109-109
| if (is_dir($dir)) { | ||
| foreach (glob($dir . '/*.csv') as $path) { | ||
| $name = pathinfo($path, PATHINFO_FILENAME); | ||
| $slug = Str::slug($name); | ||
|
|
||
| // Try to find a Company model so we can use the media library image_path. | ||
| $companyModel = Company::query()->where('slug', $slug) | ||
| ->orWhere('name', $name) | ||
| ->first(); | ||
|
|
||
| $imagePath = $companyModel?->image_path ?? asset('images/logos/' . $slug . '.svg'); | ||
|
|
||
| $files[] = [ | ||
| 'slug' => $slug, | ||
| 'name' => $name, | ||
| 'image_path' => $imagePath, | ||
| ]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Database query inside a loop — potential performance concern.
Each CSV file triggers a separate Company::query() call (lines 28–30). If the storage/app/matrix directory grows, this becomes an N+1-style problem.
Batch-load all matching companies once before the loop:
♻️ Proposed fix
if (is_dir($dir)) {
+ $csvNames = collect(glob($dir . '/*.csv'))
+ ->map(fn ($path) => pathinfo($path, PATHINFO_FILENAME));
+ $slugs = $csvNames->map(fn ($n) => Str::slug($n));
+
+ $companyModels = Company::query()
+ ->whereIn('slug', $slugs)
+ ->orWhereIn('name', $csvNames)
+ ->get()
+ ->keyBy('slug');
+
foreach (glob($dir . '/*.csv') as $path) {
$name = pathinfo($path, PATHINFO_FILENAME);
$slug = Str::slug($name);
- // Try to find a Company model so we can use the media library image_path.
- $companyModel = Company::query()->where('slug', $slug)
- ->orWhere('name', $name)
- ->first();
-
- $imagePath = $companyModel?->image_path ?? asset('images/logos/' . $slug . '.svg');
+ $companyModel = $companyModels[$slug] ?? null;
+ $imagePath = $companyModel?->image_path ?? asset('images/logos/' . $slug . '.svg');As per coding guidelines: "Prevent N+1 queries by using eager loading when accessing relations" — while this isn't a relationship traversal, the same principle of batching DB access applies.
🤖 Prompt for AI Agents
In `@app/Http/Controllers/MatrixAlternativesController.php` around lines 22 - 40,
The loop in MatrixAlternativesController reads CSV files and runs
Company::query()->where('slug', $slug)->orWhere('name', $name)->first() per file
causing N+1 DB queries; instead, before iterating over files gather all slugs
and names (from pathinfo/Str::slug), run one batched query on the Company model
to fetch all matching companies (e.g., whereIn('slug', $slugs) and
whereIn('name', $names)), build an in-memory map keyed by slug and/or name, and
then inside the foreach use that map to set $companyModel/$imagePath (falling
back to asset(...)) and populate $files — update references to $companyModel,
$slug, $name accordingly.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
resources/views/matrix/details.blade.php (1)
67-67: Redundant conditional:$selectedis always truthy here.This line is inside the
@if ($selected)guard on line 8, so$selected ? 'bg-accent/10' : ''always evaluates to the truthy branch. Simplify to a static class.Proposed fix
- <td class="p-2 {{ $selected ? 'bg-accent/10' : '' }}">{!! nl2br(e($renderCellValue($selected, $item))) !!}</td> + <td class="p-2 bg-accent/10">{!! nl2br(e($renderCellValue($selected, $item))) !!}</td>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/matrix/details.blade.php` at line 67, The conditional class expression inside the <td> element is redundant because this template branch is already within an `@if` ($selected) guard; replace the ternary expression {{ $selected ? 'bg-accent/10' : '' }} with the static class 'bg-accent/10' so the <td> uses class="p-2 bg-accent/10" while keeping the existing nl2br(e($renderCellValue($selected, $item))) rendering intact.app/Http/Controllers/MatrixAlternativesController.php (3)
648-670:enrichRowsForDisplaycallscalculateBestScoreredundantly.The
show()method on line 64 callscalculateBestScore($rows, $company)and passes it to the view. Then on line 61, it callsenrichRowsForDisplay($rows, $company)which internally callscalculateBestScoreagain (line 650) — parsing the same array twice to find the same value.Consider passing the already-computed
bestScoreintoenrichRowsForDisplayas a parameter.Proposed fix
- private function enrichRowsForDisplay(array $rows, string $company): array + private function enrichRowsForDisplay(array $rows, string $company, ?int $bestScore = null): array { - $bestScore = $this->calculateBestScore($rows, $company); + $bestScore ??= $this->calculateBestScore($rows, $company); $target = Str::slug($company);In
show():+ $bestScore = $this->calculateBestScore($rows, $company); return view('matrix.show', [ 'company' => $company, - 'rows' => $this->enrichRowsForDisplay($rows, $company), + 'rows' => $this->enrichRowsForDisplay($rows, $company, $bestScore), // ... - 'bestScore' => $this->calculateBestScore($rows, $company), + 'bestScore' => $bestScore,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/MatrixAlternativesController.php` around lines 648 - 670, The enrichRowsForDisplay method currently calls calculateBestScore internally causing redundant work; change enrichRowsForDisplay(array $rows, string $company) to accept the precomputed bestScore (e.g., enrichRowsForDisplay(array $rows, int $bestScore)) and remove the internal calculateBestScore call, then update the show() caller to compute $bestScore = $this->calculateBestScore($rows, $company) once and pass that $bestScore into enrichRowsForDisplay; ensure the closure inside enrichRowsForDisplay uses the passed $bestScore and adjust any type hints/signatures accordingly.
8-8: Consider adding PHPDoc blocks to the class and its public methods.Per coding guidelines, prefer PHPDoc blocks over inline comments. The class-level inline comments (lines 10, 19, 93, etc.) could be replaced with proper PHPDoc blocks, and the public methods should document their parameters and return types.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/MatrixAlternativesController.php` at line 8, Add PHPDoc blocks for the MatrixAlternativesController class and all its public methods (e.g., methods declared in MatrixAlternativesController) replacing the existing inline comments: add a class-level PHPDoc describing purpose and author/license if applicable, and for each public method include `@param` tags for each parameter with types, an `@return` tag with the return type (e.g., JsonResponse, Response, RedirectResponse, bool, etc.), and any `@throws` tags for exceptions the method can raise; ensure the docblocks sit immediately above the class and method declarations and follow the project's PHPDoc style conventions.
345-363:getMetricKeyMapandgetDefaultWeightsare redundant — they encode the same data.Both maps associate the same metric names with the same weight values (
Features → 25,Security and Compliance → 5, etc.).getDefaultWeightsis only used as a fallback incomputeScoresForRowsline 324 when a weight isn't found in$weightLookup. SincegetMetricKeyMapalready carries the weight, you could derive defaults from it and eliminate the duplication.Proposed fix
private function computeScoresForRows(array $rows, array $weightLookup): array { $metricKeyMap = $this->getMetricKeyMap(); - $defaultWeights = $this->getDefaultWeights(); foreach ($rows as $i => $row) { // ... foreach ($metricKeyMap as $metricName => $info) { - $weight = $weightLookup[$metricName] ?? ($defaultWeights[$metricName] ?? 0); + $weight = $weightLookup[$metricName] ?? $info['weight'];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/MatrixAlternativesController.php` around lines 345 - 363, getMetricKeyMap and getDefaultWeights duplicate the same metric→weight data; remove the duplication by deleting getDefaultWeights and change computeScoresForRows to derive fallback weights from getMetricKeyMap (use the 'weight' values inside getMetricKeyMap when a metric is missing in $weightLookup). Update computeScoresForRows to call getMetricKeyMap() once to build a $defaultWeights map (metric name → weight) and use that as the fallback where the code currently references getDefaultWeights(), and remove any other references to getDefaultWeights().resources/views/matrix/show.blade.php (2)
69-69: View contains domain logic (Str::slug) that should live in the controller.Inline
\Illuminate\Support\Str::slug($r['name'])in the Blade template couples the view to business logic. TheenrichRowsForDisplaymethod in the controller already enriches rows — consider adding aslugkey there and using it here.Proposed fix
- <a href="{{ route('matrix.details', ['alternative' => \Illuminate\Support\Str::slug($r['name']), 'company' => $company]) }}" class="...">Details</a> + <a href="{{ route('matrix.details', ['alternative' => $r['slug'], 'company' => $company]) }}" class="...">Details</a>In the controller's
enrichRowsForDisplay, add the slug:return array_merge($cleanRow, [ 'score' => $score, 'slug' => Str::slug($name), // ... ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/matrix/show.blade.php` at line 69, The Blade view is performing domain logic via \Illuminate\Support\Str::slug($r['name']); move that into the controller by adding a 'slug' key inside enrichRowsForDisplay (e.g., compute Str::slug($name) and include 'slug' => ... in the returned array) and then update the view to use the precomputed slug for the route param (use $r['slug'] instead of calling Str::slug in the template).
1-83: No dark mode support throughout the view.Same concern as the index view — all colors are hardcoded for light mode with no
dark:variants. As per coding guidelines: "Support dark mode consistently using dark: variants where the project does."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/matrix/show.blade.php` around lines 1 - 83, The view currently hardcodes light-mode Tailwind classes (e.g., bg-gray-50, text-gray-600, bg-gray-100, text-gray-500, bg-green-500, bg-green-400, border-gray-200, hover:bg-gray-50, border-gray-300) and lacks dark: variants; update the elements in matrix/show.blade.php (header h1/p, table thead/tbody/tr/td, the "Searched"/"TOP" spans, progress bar containers and bars, score text, Details link with its border/hover classes) to include appropriate dark: counterparts (for example dark:bg-gray-800/dark:bg-gray-900, dark:text-gray-200/dark:text-gray-300, dark:border-gray-700, dark:bg-gray-700 for progress bg, dark:bg-green-600 for bars, dark:hover:bg-gray-800) so the page respects dark mode while preserving the existing light classes and behavior (keep same conditional logic around $r, $rows, $columnMaxes and the route('matrix.details') link).resources/views/matrix/index.blade.php (2)
33-33: Commented-out code references undefined$slug.
$slugis not in scope inside this@foreach— only$cis. Either remove the dead comment or fix to reference$c['slug'].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/matrix/index.blade.php` at line 33, The commented-out Blade line references an undefined variable $slug inside the `@foreach` where only $c is available; either remove that dead comment or update it to use the correct variable (e.g. change $slug to $c['slug'] or $c->slug depending on how $c is structured) so the comment no longer references an out-of-scope variable; ensure you update the comment text consistently if you keep it.
1-42: No dark mode support.This view hardcodes light-mode colors (
bg-white,text-gray-600,border-gray-100, etc.) without anydark:variants. Other views in the project usedark:variants for consistent dark mode support.At minimum, consider adding
dark:counterparts to the key elements (e.g.,dark:bg-gray-800on cards,dark:text-gray-300on muted text,dark:border-gray-700on borders). As per coding guidelines: "Support dark mode similar to existing components using dark: variants."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/views/matrix/index.blade.php` around lines 1 - 42, The template currently hardcodes light-mode Tailwind classes (e.g., bg-white, text-gray-600, border-gray-100) causing no dark-mode support; update the key elements — the outer container (<main>), the search input (`#company-search`), the search button, the company card anchor (<a> with classes "flex ... bg-white"), muted text nodes (e.g., "text-sm text-gray-600", "text-sm text-gray-500", "text-gray-500"), and borders — to include corresponding dark: variants (for example dark:bg-gray-800 on cards, dark:text-gray-300 on muted text, dark:border-gray-700 on borders, dark:bg-gray-700 for inputs, dark:text-white for primary text) so the elements named in the diff (x-app-layout main, input id="company-search", button, the <a> card elements, and the various text spans) render correctly in dark mode consistent with the project's other views.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@resources/views/matrix/details.blade.php`:
- Line 52: The template accesses $searchedCompany['name'] directly which will
throw if findRowByName returned null; update the blade to guard against null by
using the null-coalescing fallback used elsewhere (e.g. $searchedCompany['name']
?? $company ?? '') so the heading renders a safe string when $searchedCompany is
null; locate the usage of $searchedCompany in the <th> (and keep consistency
with lines using $company and empty-string fallbacks).
In `@resources/views/matrix/index.blade.php`:
- Line 23: Replace the raw company name used in the route parameter with the
slug to ensure URL-safe links: update the route call that currently uses
route('matrix.show', ['company' => $c['name']]) to use the available $c['slug']
value instead so the generated URL is slug-encoded and won't break with spaces
or special characters.
In `@resources/views/matrix/show.blade.php`:
- Around line 50-65: The current cast (int)$r[$col] treats non-numeric strings
like "Yes" as 0 and renders a misleading progress bar; update the assignment for
$val in the foreach so it only becomes an integer when the source is numeric
(e.g. use is_numeric($r[$col]) or filter_var with FILTER_VALIDATE_INT) otherwise
set $val = null, keep $max = $columnMaxes[$col] ?? 100, and keep the subsequent
$val !== null check to render the "—" fallback for non-numeric values; modify
the block that computes $val near the foreach over
['features','security','pricing','islPresence'] to perform this numeric
validation before casting.
---
Duplicate comments:
In `@app/Http/Controllers/MatrixAlternativesController.php`:
- Around line 95-117: The resolveCsvFile method currently concatenates the raw
$company into a path and uses permissive substring matches; change it to
sanitize and slugify the input first and only check for exact slug matches to
avoid path traversal and accidental short-substring hits: compute $companySlug =
Str::slug($company), build the direct path from that slug (e.g. $dir . '/' .
$companySlug . '.csv') and check file_exists on that path (do not use the raw
$company); when scanning files with glob($dir . '/*.csv') normalize each
candidate name via $candidateSlug = Str::slug(pathinfo($path,
PATHINFO_FILENAME)) and only return $path if $candidateSlug === $companySlug
(remove Str::contains checks).
- Around line 21-48: Update MatrixAlternativesController's public method
signatures to include explicit return types: change index() to declare a return
type matching its possible returns
(Illuminate\View\View|\Illuminate\Http\RedirectResponse), and update show(...)
and details(...) to declare the concrete return types they actually return
(e.g., Illuminate\View\View, Illuminate\Http\JsonResponse, or
Illuminate\Http\RedirectResponse as applicable). Use either FQCNs in the
signatures or add use statements for the chosen types at the top of the file,
making sure each public method's signature precisely matches the types returned
inside the method body (index, show, details).
- Around line 30-44: The loop over CSV files performs an N+1 query via
Company::query() for each file; instead, first collect all file slugs and names
(from pathinfo + Str::slug) into arrays, run a single batch query like
Company::whereIn('slug', $slugs)->orWhereIn('name', $names)->get(), then index
the result (e.g., keyBy('slug') and also map by name) and inside the foreach
replace the per-iteration Company::query() lookup with a lookup into that
in-memory map when building $files (preserving the existing
'slug','name','image_path' logic using the mapped model if present). Ensure you
still fall back to asset('images/logos/'.$slug.'.svg') when no model is found.
- Around line 530-534: The current bidirectional partial-match logic using
str_contains($k, $normKey) || str_contains($normKey, $k) can return the wrong
key; change the loop to first check for an exact match ($k === $normKey) and
return that immediately; if no exact match is found, look for keys that end with
the normKey using str_ends_with($k, $normKey) but select the longest matching
key (track the best match and return it after the loop) to make the lookup
deterministic; update references to normMap and normKey in the foreach loop and
remove the original bidirectional str_contains check.
In `@resources/views/matrix/details.blade.php`:
- Around line 20-22: The anchor renders unvalidated user input via href="{{
$selected['website'] }}", allowing unsafe schemes (e.g., javascript:, data:) —
update the code that provides $selected['website'] (or validate in the Blade
before rendering) to only allow safe schemes (http, https, maybe ftp) and
normalize/strip or reject anything else; implement a whitelist check (e.g.,
parse the URL scheme and only render the <a> when it matches allowed schemes)
and ensure you escape/encode the final URL, or replace with a safe fallback or
remove the link if validation fails (refer to the $selected['website'] usage in
the Blade and the controller/method that populates $selected).
In `@resources/views/matrix/show.blade.php`:
- Line 37: The span element uses an invalid Tailwind class "text-white-700";
update the element's class list (the span with the "TOP" label) to replace
"text-white-700" with the correct "text-white" so the text color uses the valid
Tailwind v3 utility; ensure no other color shade suffix is applied to "white" in
that same class attribute.
---
Nitpick comments:
In `@app/Http/Controllers/MatrixAlternativesController.php`:
- Around line 648-670: The enrichRowsForDisplay method currently calls
calculateBestScore internally causing redundant work; change
enrichRowsForDisplay(array $rows, string $company) to accept the precomputed
bestScore (e.g., enrichRowsForDisplay(array $rows, int $bestScore)) and remove
the internal calculateBestScore call, then update the show() caller to compute
$bestScore = $this->calculateBestScore($rows, $company) once and pass that
$bestScore into enrichRowsForDisplay; ensure the closure inside
enrichRowsForDisplay uses the passed $bestScore and adjust any type
hints/signatures accordingly.
- Line 8: Add PHPDoc blocks for the MatrixAlternativesController class and all
its public methods (e.g., methods declared in MatrixAlternativesController)
replacing the existing inline comments: add a class-level PHPDoc describing
purpose and author/license if applicable, and for each public method include
`@param` tags for each parameter with types, an `@return` tag with the return type
(e.g., JsonResponse, Response, RedirectResponse, bool, etc.), and any `@throws`
tags for exceptions the method can raise; ensure the docblocks sit immediately
above the class and method declarations and follow the project's PHPDoc style
conventions.
- Around line 345-363: getMetricKeyMap and getDefaultWeights duplicate the same
metric→weight data; remove the duplication by deleting getDefaultWeights and
change computeScoresForRows to derive fallback weights from getMetricKeyMap (use
the 'weight' values inside getMetricKeyMap when a metric is missing in
$weightLookup). Update computeScoresForRows to call getMetricKeyMap() once to
build a $defaultWeights map (metric name → weight) and use that as the fallback
where the code currently references getDefaultWeights(), and remove any other
references to getDefaultWeights().
In `@resources/views/matrix/details.blade.php`:
- Line 67: The conditional class expression inside the <td> element is redundant
because this template branch is already within an `@if` ($selected) guard; replace
the ternary expression {{ $selected ? 'bg-accent/10' : '' }} with the static
class 'bg-accent/10' so the <td> uses class="p-2 bg-accent/10" while keeping the
existing nl2br(e($renderCellValue($selected, $item))) rendering intact.
In `@resources/views/matrix/index.blade.php`:
- Line 33: The commented-out Blade line references an undefined variable $slug
inside the `@foreach` where only $c is available; either remove that dead comment
or update it to use the correct variable (e.g. change $slug to $c['slug'] or
$c->slug depending on how $c is structured) so the comment no longer references
an out-of-scope variable; ensure you update the comment text consistently if you
keep it.
- Around line 1-42: The template currently hardcodes light-mode Tailwind classes
(e.g., bg-white, text-gray-600, border-gray-100) causing no dark-mode support;
update the key elements — the outer container (<main>), the search input
(`#company-search`), the search button, the company card anchor (<a> with classes
"flex ... bg-white"), muted text nodes (e.g., "text-sm text-gray-600", "text-sm
text-gray-500", "text-gray-500"), and borders — to include corresponding dark:
variants (for example dark:bg-gray-800 on cards, dark:text-gray-300 on muted
text, dark:border-gray-700 on borders, dark:bg-gray-700 for inputs,
dark:text-white for primary text) so the elements named in the diff
(x-app-layout main, input id="company-search", button, the <a> card elements,
and the various text spans) render correctly in dark mode consistent with the
project's other views.
In `@resources/views/matrix/show.blade.php`:
- Line 69: The Blade view is performing domain logic via
\Illuminate\Support\Str::slug($r['name']); move that into the controller by
adding a 'slug' key inside enrichRowsForDisplay (e.g., compute Str::slug($name)
and include 'slug' => ... in the returned array) and then update the view to use
the precomputed slug for the route param (use $r['slug'] instead of calling
Str::slug in the template).
- Around line 1-83: The view currently hardcodes light-mode Tailwind classes
(e.g., bg-gray-50, text-gray-600, bg-gray-100, text-gray-500, bg-green-500,
bg-green-400, border-gray-200, hover:bg-gray-50, border-gray-300) and lacks
dark: variants; update the elements in matrix/show.blade.php (header h1/p, table
thead/tbody/tr/td, the "Searched"/"TOP" spans, progress bar containers and bars,
score text, Details link with its border/hover classes) to include appropriate
dark: counterparts (for example dark:bg-gray-800/dark:bg-gray-900,
dark:text-gray-200/dark:text-gray-300, dark:border-gray-700, dark:bg-gray-700
for progress bg, dark:bg-green-600 for bars, dark:hover:bg-gray-800) so the page
respects dark mode while preserving the existing light classes and behavior
(keep same conditional logic around $r, $rows, $columnMaxes and the
route('matrix.details') link).
| @foreach (['features','security','pricing','islPresence'] as $col) | ||
| @php | ||
| $val = isset($r[$col]) ? (int)$r[$col] : null; | ||
| $max = $columnMaxes[$col] ?? 100; | ||
| @endphp | ||
| <td class="px-3 py-3 align-middle"> | ||
| @if ($val !== null) | ||
| <div class="w-40 bg-gray-100 h-2 rounded overflow-hidden"> | ||
| <div class="h-2 bg-green-500" style="width: {{ ($val/$max)*100 }}%;"></div> | ||
| </div> | ||
| <div class="text-xs text-gray-500 mt-1">{{ $val }}/{{ $max }}</div> | ||
| @else | ||
| <div class="text-sm text-gray-500">—</div> | ||
| @endif | ||
| </td> | ||
| @endforeach |
There was a problem hiding this comment.
Casting non-numeric strings to (int) silently produces 0, which is then rendered as a valid progress bar.
Line 52 casts $r[$col] to (int) whenever isset. If the CSV cell contains a non-numeric string like "Yes" or "N/A", (int) yields 0, and $val !== null passes — resulting in a misleading 0-width progress bar instead of the — fallback.
Proposed fix
`@php`
- $val = isset($r[$col]) ? (int)$r[$col] : null;
+ $val = isset($r[$col]) && is_numeric($r[$col]) ? (int)$r[$col] : null;
$max = $columnMaxes[$col] ?? 100;
`@endphp`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/views/matrix/show.blade.php` around lines 50 - 65, The current cast
(int)$r[$col] treats non-numeric strings like "Yes" as 0 and renders a
misleading progress bar; update the assignment for $val in the foreach so it
only becomes an integer when the source is numeric (e.g. use
is_numeric($r[$col]) or filter_var with FILTER_VALIDATE_INT) otherwise set $val
= null, keep $max = $columnMaxes[$col] ?? 100, and keep the subsequent $val !==
null check to render the "—" fallback for non-numeric values; modify the block
that computes $val near the foreach over
['features','security','pricing','islPresence'] to perform this numeric
validation before casting.
Also adds sorting based on total score
Json-To-String
changed the title
1 participant
Tackles #183 and #184
Still needs:
Summary by CodeRabbit