[4.x] QoL & Polish#1145
Merged
RichardAnderson merged 2 commits intoJun 5, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR refactors breadcrumb navigation by replacing a shared Breadcrumbs component with a new BreadcrumbHeader component rendered per-page, refactors SSH users to a top-level Inertia prop, adds service handler validation, standardizes Card styling across the application with overflow-hidden and bg-background classes, and adopts DataTable for metrics rendering. ChangesBreadcrumb Navigation and Layout Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
tests/Feature/ServiceLogsTest.php (1)
54-58: ⚡ Quick winMake the new test deterministic by setting service status explicitly.
This test currently relies on factory defaults to include the new service in the
READY-only query. SetstatustoServiceStatus::READYin the factory payload so the test always exercises thehasHandler()skip path directly.Proposed change
use App\Facades\SSH; +use App\Enums\ServiceStatus; use App\Models\Service; @@ Service::factory()->create([ 'server_id' => $this->server->id, 'type' => 'vpn', 'name' => 'wireguard', + 'status' => ServiceStatus::READY, ]);🤖 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/ServiceLogsTest.php` around lines 54 - 58, The test is non-deterministic because the Service factory relies on defaults; update the Service::factory()->create call to explicitly set the service status to ServiceStatus::READY (e.g., include 'status' => ServiceStatus::READY in the payload) so the created record will always match the READY-only query and exercise the hasHandler() skip path; ensure the ServiceStatus symbol is imported or fully qualified in the test file.resources/js/pages/monitoring/components/memory-disk-details.tsx (1)
35-49: 💤 Low valueConsider memoizing the column definition.
The
columnsarray is recreated on every render. For small datasets this has negligible impact, but you can optimize by moving the column factory outside or wrapping inuseMemo:♻️ Optional optimization
+const createDetailsColumns = (title: string): ColumnDef<DetailRow>[] => [ + { + accessorKey: 'label', + header: title, + }, + { + accessorKey: 'value', + header: '', + cell: ({ row }) => <div className="text-right">{row.original.value}</div>, + }, +]; + function DetailsTable({ title, rows }: { title: string; rows: DetailRow[] }) { - const columns: ColumnDef<DetailRow>[] = [ - { - accessorKey: 'label', - header: title, - }, - { - accessorKey: 'value', - header: '', - cell: ({ row }) => <div className="text-right">{row.original.value}</div>, - }, - ]; + const columns = useMemo(() => createDetailsColumns(title), [title]); return <DataTable columns={columns} data={rows} />; }🤖 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 `@resources/js/pages/monitoring/components/memory-disk-details.tsx` around lines 35 - 49, The columns array in DetailsTable is recreated on every render; wrap the columns definition in React.useMemo (or move the factory outside the component) so it's stable across renders — e.g., in the DetailsTable function memoize the columns used by DataTable with useMemo and include the proper dependency(s) (at minimum the title prop) to avoid unnecessary re-creation; reference the columns variable and the DetailsTable component and ensure the cell renderer remains unchanged.resources/js/pages/sites/stats.tsx (2)
326-344: 💤 Low valueConsider memoizing or extracting the column definition.
The
columnsarray is recreated on every render. While the performance impact is negligible for these small datasets, you can optimize by extracting the definition:♻️ Optional optimization
+const panelColumns: ColumnDef<SiteStatsPanelRow>[] = [ + { + accessorKey: 'name', + header: 'Page', + cell: ({ row }) => <div className="max-w-[280px] truncate">{row.original.name}</div>, + }, + { + accessorKey: 'hits', + header: () => <div className="text-right">Hits</div>, + cell: ({ row }) => <div className="text-right">{row.original.hits.toLocaleString()}</div>, + }, + { + accessorKey: 'visitors', + header: () => <div className="text-right">Visitors</div>, + cell: ({ row }) => <div className="text-right">{row.original.visitors.toLocaleString()}</div>, + }, +]; + +function PanelCard({ title, rows }: { title: string; rows: SiteStatsPanelRow[] }) { + const columns = useMemo( + (): ColumnDef<SiteStatsPanelRow>[] => [ + { ...panelColumns[0], header: title }, + panelColumns[1], + panelColumns[2], + ], + [title], + ); - const columns: ColumnDef<SiteStatsPanelRow>[] = [ - { - accessorKey: 'name', - header: title, - cell: ({ row }) => <div className="max-w-[280px] truncate">{row.original.name}</div>, - }, - { - accessorKey: 'hits', - header: () => <div className="text-right">Hits</div>, - cell: ({ row }) => <div className="text-right">{row.original.hits.toLocaleString()}</div>, - }, - { - accessorKey: 'visitors', - header: () => <div className="text-right">Visitors</div>, - cell: ({ row }) => <div className="text-right">{row.original.visitors.toLocaleString()}</div>, - }, - ]; return <DataTable columns={columns} data={rows} />; }🤖 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 `@resources/js/pages/sites/stats.tsx` around lines 326 - 344, The columns array (ColumnDef<SiteStatsPanelRow>[] named columns) is recreated on every render which can be avoided; wrap the columns definition in a useMemo (or extract it outside the component) so it is only recomputed when its dependencies change (e.g., title), e.g., memoize the columns used by DataTable to improve render stability and avoid re-creating cell/header functions on each render; ensure useMemo is imported and include title (and any other props used in the column definitions) in the dependency array.
348-360: 💤 Low valueConsider extracting the column definition outside the component.
The
columnsarray is recreated on every render. For a minor performance improvement, extract it as a constant:♻️ Optional optimization
+const statusCodeColumns: ColumnDef<SiteStatsStatusCode>[] = [ + { + accessorKey: 'name', + header: 'Status code', + }, + { + accessorKey: 'hits', + header: () => <div className="text-right">Hits</div>, + cell: ({ row }) => <div className="text-right">{row.original.hits.toLocaleString()}</div>, + }, +]; + function StatusCodesCard({ rows }: { rows: SiteStatsStatusCode[] }) { - const columns: ColumnDef<SiteStatsStatusCode>[] = [ - { - accessorKey: 'name', - header: 'Status code', - }, - { - accessorKey: 'hits', - header: () => <div className="text-right">Hits</div>, - cell: ({ row }) => <div className="text-right">{row.original.hits.toLocaleString()}</div>, - }, - ]; - return <DataTable columns={columns} data={rows} />; + return <DataTable columns={statusCodeColumns} data={rows} />; }🤖 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 `@resources/js/pages/sites/stats.tsx` around lines 348 - 360, The columns array (const columns: ColumnDef<SiteStatsStatusCode>[]) is being recreated on every render; move that definition out of the component and into a module-level constant (e.g., COLUMNS or SITE_STATS_COLUMNS) typed as ColumnDef<SiteStatsStatusCode>[] and then reference it in the component when rendering <DataTable columns={COLUMNS} data={rows} />; ensure any JSX header/cell callbacks remain valid at module scope (or wrap dynamic parts with useMemo if they depend on props/state).
🤖 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 `@app/Http/Resources/ServerResource.php`:
- Line 25: ServerResource is exposing unfiltered SSH users by calling
Server::getSshUsers() (which always includes root); change
ServerResource::ssh_users to call Server::sshLoginUsers() (or a shared helper
that applies the same root-login gating as sshLoginUsers()) so the
Inertia-shared page.props.server no longer exposes root when root login is
disabled. Also update backend validation and actions that currently use
$server->getSshUsers() — specifically replace Rule::in($server->getSshUsers())
in RunCommand and the default/assignment of $users in ExecuteScript to use
$server->sshLoginUsers() (or the same shared helper) so input validation and
script execution respect the root-login feature flag.
In `@resources/js/pages/site-tooling/index.tsx`:
- Around line 66-71: The anchor element wrapping the Button (the external link
to "https://vitodeploy.com/docs/sites/tooling") uses target="_blank" but lacks
the rel attribute; update that <a> element to include rel="noopener noreferrer"
so the external page cannot access window.opener and to follow security best
practices while leaving the existing Button and BookOpenIcon usage unchanged.
In `@resources/js/pages/sites/stats.tsx`:
- Around line 303-312: In DocsButton, avoid nesting interactive elements by
rendering the Button as the anchor using the Button's asChild prop (replace the
outer <a> + inner <Button> with a single Button asChild that contains the
BookOpenIcon and label), keep target="_blank" on the anchor element produced and
add rel="noopener noreferrer" for security; update the DocsButton function to
return a Button rendered as the link (using asChild) with the external link
attributes and existing content.
In `@resources/js/pages/workflow-runs/show.tsx`:
- Line 42: Breadcrumb title uses `History of ${page.props.workflow.name}` but
the <Head> title and the rendered <Heading> still use the old `"Workflow
[${workflow.name}]"`; update the <Head> element's title and the Heading's title
to match the breadcrumb (use `History of ${page.props.workflow.name}`) so all
three display the same string, locating the strings in the component that
references page.props.workflow and the Heading component rendering the page
title.
---
Nitpick comments:
In `@resources/js/pages/monitoring/components/memory-disk-details.tsx`:
- Around line 35-49: The columns array in DetailsTable is recreated on every
render; wrap the columns definition in React.useMemo (or move the factory
outside the component) so it's stable across renders — e.g., in the DetailsTable
function memoize the columns used by DataTable with useMemo and include the
proper dependency(s) (at minimum the title prop) to avoid unnecessary
re-creation; reference the columns variable and the DetailsTable component and
ensure the cell renderer remains unchanged.
In `@resources/js/pages/sites/stats.tsx`:
- Around line 326-344: The columns array (ColumnDef<SiteStatsPanelRow>[] named
columns) is recreated on every render which can be avoided; wrap the columns
definition in a useMemo (or extract it outside the component) so it is only
recomputed when its dependencies change (e.g., title), e.g., memoize the columns
used by DataTable to improve render stability and avoid re-creating cell/header
functions on each render; ensure useMemo is imported and include title (and any
other props used in the column definitions) in the dependency array.
- Around line 348-360: The columns array (const columns:
ColumnDef<SiteStatsStatusCode>[]) is being recreated on every render; move that
definition out of the component and into a module-level constant (e.g., COLUMNS
or SITE_STATS_COLUMNS) typed as ColumnDef<SiteStatsStatusCode>[] and then
reference it in the component when rendering <DataTable columns={COLUMNS}
data={rows} />; ensure any JSX header/cell callbacks remain valid at module
scope (or wrap dynamic parts with useMemo if they depend on props/state).
In `@tests/Feature/ServiceLogsTest.php`:
- Around line 54-58: The test is non-deterministic because the Service factory
relies on defaults; update the Service::factory()->create call to explicitly set
the service status to ServiceStatus::READY (e.g., include 'status' =>
ServiceStatus::READY in the payload) so the created record will always match the
READY-only query and exercise the hasHandler() skip path; ensure the
ServiceStatus symbol is imported or fully qualified in the test file.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b0be647-25aa-4e5c-b669-8c0f878857e6
📒 Files selected for processing (32)
app/Actions/Server/GetServiceLogs.phpapp/Http/Controllers/ConsoleController.phpapp/Http/Controllers/CronJobController.phpapp/Http/Resources/ServerResource.phpapp/Models/Service.phpresources/js/components/breadcrumb-header.tsxresources/js/components/breadcrumbs.tsxresources/js/layouts/admin/layout.tsxresources/js/layouts/app/layout.tsxresources/js/layouts/settings/layout.tsxresources/js/pages/commands/show.tsxresources/js/pages/domains/show.tsxresources/js/pages/github-app/index.tsxresources/js/pages/monitoring/components/memory-disk-details.tsxresources/js/pages/plugins/components/official.tsxresources/js/pages/plugins/index.tsxresources/js/pages/scripts/index.tsxresources/js/pages/scripts/show.tsxresources/js/pages/security/index.tsxresources/js/pages/server-features/index.tsxresources/js/pages/server-logs/services.tsxresources/js/pages/server-settings/index.tsxresources/js/pages/servers/console.tsxresources/js/pages/site-features/index.tsxresources/js/pages/site-settings/index.tsxresources/js/pages/site-tooling/components/tooling-table.tsxresources/js/pages/site-tooling/index.tsxresources/js/pages/sites/stats.tsxresources/js/pages/workflow-runs/index.tsxresources/js/pages/workflow-runs/show.tsxtests/Feature/ServiceLogsTest.phpvite.config.ts
💤 Files with no reviewable changes (2)
- resources/js/components/breadcrumbs.tsx
- app/Http/Controllers/CronJobController.php
saeedvaziry
approved these changes
Jun 5, 2026
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.
BreadcrumbHeadercomponent for consistent breadcrumb navigation with a back button, and updated pages such ascommands/show.tsxanddomains/show.tsxto use it.Breadcrumbscomponent and refactored layout components (app/layout.tsx,settings/layout.tsx,admin/layout.tsx) to no longer require or pass breadcrumb props, simplifying their interfaces.CardHeader,CardTitle, andCardDescriptionfor better structure, and added separators between installation rows for clarity.hasHandler()method to theServicemodel and updated log retrieval logic to skip services without a handler, making service log handling more robust.sshLoginUsers()withgetSshUsers()in resources and controllers, ensuring consistency in how SSH users are fetched and exposed.npm run deverrors and warningsSummary by CodeRabbit
New Features
Bug Fixes
UI/Style
Tests
Chores