From d5872917c3834987d37070b347d655a9f2247780 Mon Sep 17 00:00:00 2001 From: Eric Wang <37554696+ericwang401@users.noreply.github.com> Date: Fri, 16 Oct 2026 00:43:58 +0000 Subject: [PATCH 1/2] fix: show "not found" for deleted servers, nodes and users Opening a server that had been deleted showed "Something went wrong" with Laravel's "No query results for model [Convoy\Models\Server]": the backend sent that message as the 404's body and the server routers put any error's message on a generic error screen. Admin node and user pages were worse: their loaders swallowed the error and returned null, and the routers then read node.name / user.name and broke. Backend: a renderable on NotFoundHttpException (Laravel has converted the ModelNotFoundException by then) answers JSON requests with "This could not be found. It may have been deleted." Frontend: a 404 now shows the existing NotFound screen -- "Server not found" / "Node not found" / "User not found" -- with Go Back to the list. Server routers check isNotFoundError() on the fetch; the node and user loaders return a LoadFailure instead of null, which their routers render through LoadFailureScreen before touching the record. Other errors still use the generic error screen, now with a readable message. Tested with Pest (the API message for a missing server and node) and a Playwright spec opening a missing server (client and admin), node and user; all four fail on the old code. --- CHANGELOG.md | 9 ++++ app/Exceptions/Handler.php | 23 +++++++++ resources/scripts/api/http.ts | 7 +++ resources/scripts/routers/AdminNodeRouter.tsx | 25 ++++++++-- .../scripts/routers/AdminServerRouter.tsx | 21 ++++++-- resources/scripts/routers/AdminUserRouter.tsx | 27 +++++++++-- resources/scripts/routers/ServerRouter.tsx | 21 ++++++-- resources/scripts/routers/helpers.tsx | 48 ++++++++++++++++++- tests/Feature/NotFoundResponseTest.php | 19 ++++++++ tests/e2e/admin/not-found.spec.ts | 24 ++++++++++ 10 files changed, 206 insertions(+), 18 deletions(-) create mode 100644 tests/Feature/NotFoundResponseTest.php create mode 100644 tests/e2e/admin/not-found.spec.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index deb96919c74..6264038dac2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,15 @@ This file is a running track of new features and fixes to each version of the pa The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project follows [Semantic Versioning](https://semver.org) guidelines. +## Unreleased + +### Fixed + +- Fixed opening a server that has been deleted showing "Something went wrong" with "No query results for model + [Convoy\Models\Server]". It now shows "Server not found" with a way back to the server list, and the API names a + missing record in plain words ("This server could not be found. It may have been deleted."). Deleted nodes and users + in the admin area, which used to break the page outright, now get the same screen. + ## v4.7.0-rc.2 ### Fixed diff --git a/app/Exceptions/Handler.php b/app/Exceptions/Handler.php index d610a11bcd0..4c579b6e732 100644 --- a/app/Exceptions/Handler.php +++ b/app/Exceptions/Handler.php @@ -4,8 +4,12 @@ use Exception; use Throwable; +use Illuminate\Http\Request; +use Illuminate\Support\Str; use Illuminate\Container\Container; +use Illuminate\Database\Eloquent\ModelNotFoundException; use Illuminate\Foundation\Exceptions\Handler as ExceptionHandler; +use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; class Handler extends ExceptionHandler { @@ -28,6 +32,25 @@ public function register(): void $this->reportable(function (Throwable $e) { // }); + + // A missing route-bound model came back with Laravel's own message -- + // "No query results for model [Convoy\Models\Server]" -- which the panel + // showed as-is. Name the record in plain words instead. Laravel has + // already turned it into a NotFoundHttpException by the time this runs, + // so the model is on the original exception. + $this->renderable(function (NotFoundHttpException $e, Request $request) { + $missing = $e->getPrevious(); + + if (! $missing instanceof ModelNotFoundException || ! $request->expectsJson()) { + return null; + } + + $model = Str::of(class_basename($missing->getModel()))->headline()->lower(); + + return response()->json([ + 'message' => "This {$model} could not be found. It may have been deleted.", + ], 404); + }); } /** diff --git a/resources/scripts/api/http.ts b/resources/scripts/api/http.ts index 6d6f886a8ef..9bdf06f444e 100644 --- a/resources/scripts/api/http.ts +++ b/resources/scripts/api/http.ts @@ -64,6 +64,13 @@ export default http * Converts an error into a human readable response. Mostly just a generic helper to * make sure we display the message from the server back to the user if we can. */ +/** + * Whether a request failed because the record it asked for doesn't exist -- + * typically one deleted since the page was opened. + */ +export const isNotFoundError = (error: any): boolean => + error?.response?.status === 404 + export function httpErrorToHuman(error: any): string { if (error.response && error.response.data) { let {data} = error.response diff --git a/resources/scripts/routers/AdminNodeRouter.tsx b/resources/scripts/routers/AdminNodeRouter.tsx index ebbe9fbe8ef..88ea386b8bc 100644 --- a/resources/scripts/routers/AdminNodeRouter.tsx +++ b/resources/scripts/routers/AdminNodeRouter.tsx @@ -1,8 +1,14 @@ -import { lazyLoad, query } from '@/routers/helpers' +import { + LoadFailureScreen, + isLoadFailure, + lazyLoad, + loadFailure, + query, +} from '@/routers/helpers' import { Route } from '@/routers/router' import { lazy, useContext, useEffect } from 'react' import { useTranslation } from 'react-i18next' -import { Outlet } from 'react-router-dom' +import { Outlet, useLoaderData } from 'react-router-dom' import getNode from '@/api/admin/nodes/getNode' import useNodeSWR, { getKey as getPoolKey } from '@/api/admin/nodes/useNodeSWR' @@ -31,7 +37,7 @@ export const routes: Route[] = [ const node = await query(getPoolKey(nodeId), () => getNode(nodeId)); return node; } catch (error) { - return null; + return loadFailure(error) } }, element: lazyLoad( @@ -126,6 +132,7 @@ export const routes: Route[] = [ const AdminNodeRouter = () => { const { t: tStrings } = useTranslation('strings') const { data: node } = useNodeSWR() + const loaded = useLoaderData() const routes = [ { @@ -164,9 +171,19 @@ const AdminNodeRouter = () => { }, []) useEffect(() => { - setBreadcrumb(node.name) + setBreadcrumb(node?.name) }, [node]) + if (isLoadFailure(loaded)) { + return ( + + ) + } + return <>{!node ? : } } diff --git a/resources/scripts/routers/AdminServerRouter.tsx b/resources/scripts/routers/AdminServerRouter.tsx index c178957569b..a6b0e3f97e5 100644 --- a/resources/scripts/routers/AdminServerRouter.tsx +++ b/resources/scripts/routers/AdminServerRouter.tsx @@ -4,11 +4,14 @@ import { AdminServerContext } from '@/state/admin/server' import { ExclamationCircleIcon } from '@heroicons/react/24/outline' import { lazy, useContext, useEffect, useState } from 'react' import { useTranslation } from 'react-i18next' -import { Outlet, useMatch } from 'react-router-dom' +import { Outlet, useMatch, useNavigate } from 'react-router-dom' -import { httpErrorToHuman } from '@/api/http' +import { httpErrorToHuman, isNotFoundError } from '@/api/http' -import ScreenBlock, { ErrorMessage } from '@/components/elements/ScreenBlock' +import ScreenBlock, { + ErrorMessage, + NotFound, +} from '@/components/elements/ScreenBlock' import Spinner from '@/components/elements/Spinner' import { NavigationBarContext } from '@/components/elements/navigation/NavigationBar' @@ -94,6 +97,8 @@ export const routes: Route[] = [ const AdminServerRouter = () => { const match = useMatch('/admin/servers/:id/*') const [error, setError] = useState() + const [notFound, setNotFound] = useState(false) + const navigate = useNavigate() const server = AdminServerContext.useStoreState(state => state.server.data) const getServer = AdminServerContext.useStoreActions( actions => actions.server.getServer @@ -117,8 +122,10 @@ const AdminServerRouter = () => { useEffect(() => { setError(undefined) + setNotFound(false) getServer(match!.params.id as string).catch((error: any) => { + setNotFound(isNotFoundError(error)) setError(httpErrorToHuman(error)) }) @@ -142,7 +149,13 @@ const AdminServerRouter = () => { return ( <> {!server ? ( - error ? ( + notFound ? ( + navigate('/admin/servers')} + /> + ) : error ? ( ) : ( diff --git a/resources/scripts/routers/AdminUserRouter.tsx b/resources/scripts/routers/AdminUserRouter.tsx index 702356fd951..edfb5c55881 100644 --- a/resources/scripts/routers/AdminUserRouter.tsx +++ b/resources/scripts/routers/AdminUserRouter.tsx @@ -1,7 +1,13 @@ -import { lazyLoad, query } from '@/routers/helpers' +import { + LoadFailureScreen, + isLoadFailure, + lazyLoad, + loadFailure, + query, +} from '@/routers/helpers' import { Route } from '@/routers/router' import { lazy, useContext, useEffect } from 'react' -import { Outlet } from 'react-router-dom' +import { Outlet, useLoaderData } from 'react-router-dom' import getUser from '@/api/admin/users/getUser' import useUserSWR, { getKey } from '@/api/admin/users/useUserSWR' @@ -30,7 +36,7 @@ export const routes: Route[] = [ const user = await query(getKey(id), () => getUser(id)); return user; } catch (error) { - return null; + return loadFailure(error) } }, children: [ @@ -89,6 +95,7 @@ const visibleRoutes = [ const AdminUserRouter = () => { const { setRoutes, setBreadcrumb } = useContext(NavigationBarContext) const { data: user } = useUserSWR() + const loaded = useLoaderData() useEffect(() => { setRoutes(visibleRoutes) @@ -97,8 +104,18 @@ const AdminUserRouter = () => { }, []) useEffect(() => { - setBreadcrumb(user.name) - }, [user.name]) + setBreadcrumb(user?.name) + }, [user?.name]) + + if (isLoadFailure(loaded)) { + return ( + + ) + } return } diff --git a/resources/scripts/routers/ServerRouter.tsx b/resources/scripts/routers/ServerRouter.tsx index f1a51f96e8a..627b08f1ff7 100644 --- a/resources/scripts/routers/ServerRouter.tsx +++ b/resources/scripts/routers/ServerRouter.tsx @@ -8,12 +8,15 @@ import { } from '@heroicons/react/24/outline' import { lazy, useContext, useEffect, useMemo, useState } from 'react' import { useTranslation } from 'react-i18next' -import { Outlet, useMatches } from 'react-router-dom' +import { Outlet, useMatches, useNavigate } from 'react-router-dom' -import { httpErrorToHuman } from '@/api/http' +import { httpErrorToHuman, isNotFoundError } from '@/api/http' import { EloquentStatus } from '@/api/server/types' -import ScreenBlock, { ErrorMessage } from '@/components/elements/ScreenBlock' +import ScreenBlock, { + ErrorMessage, + NotFound, +} from '@/components/elements/ScreenBlock' import Spinner from '@/components/elements/Spinner' import { NavigationBarContext } from '@/components/elements/navigation/NavigationBar' @@ -128,6 +131,8 @@ const ServerRouter = () => { const matches = useMatches() const id = matches[0].params.id const [error, setError] = useState() + const [notFound, setNotFound] = useState(false) + const navigate = useNavigate() const server = ServerContext.useStoreState(state => state.server.data) const getServer = ServerContext.useStoreActions( actions => actions.server.getServer @@ -157,8 +162,10 @@ const ServerRouter = () => { useEffect(() => { setError(undefined) + setNotFound(false) getServer(id as string).catch((error: any) => { + setNotFound(isNotFoundError(error)) setError(httpErrorToHuman(error)) }) @@ -243,7 +250,13 @@ const ServerRouter = () => { return ( <> {!server ? ( - error ? ( + notFound ? ( + navigate('/')} + /> + ) : error ? ( ) : ( diff --git a/resources/scripts/routers/helpers.tsx b/resources/scripts/routers/helpers.tsx index 4f5938e90c5..9fc75e9d6ed 100644 --- a/resources/scripts/routers/helpers.tsx +++ b/resources/scripts/routers/helpers.tsx @@ -1,6 +1,10 @@ import { LazyExoticComponent } from 'react' +import { useNavigate } from 'react-router-dom' import { BareFetcher, Key, MutatorOptions, mutate } from 'swr' +import { httpErrorToHuman, isNotFoundError } from '@/api/http' + +import { ErrorMessage, NotFound } from '@/components/elements/ScreenBlock' import Spinner from '@/components/elements/Spinner' export const lazyLoad = ( @@ -23,4 +27,46 @@ export const query = async ( await mutate(key, data, options) return data -} \ No newline at end of file +} + +/** + * What a record page's loader returns when it couldn't load the record, so the + * page can say so instead of rendering without it. + */ +export interface LoadFailure { + loadFailed: true + notFound: boolean + message: string +} + +export const loadFailure = (error: any): LoadFailure => ({ + loadFailed: true, + notFound: isNotFoundError(error), + message: httpErrorToHuman(error), +}) + +export const isLoadFailure = (data: unknown): data is LoadFailure => + typeof data === 'object' && data !== null && 'loadFailed' in data + +export const LoadFailureScreen = ({ + failure, + record, + backTo, +}: { + failure: LoadFailure + /** What the page shows, as in "Node not found". */ + record: string + backTo: string +}) => { + const navigate = useNavigate() + + return failure.notFound ? ( + navigate(backTo)} + /> + ) : ( + + ) +} diff --git a/tests/Feature/NotFoundResponseTest.php b/tests/Feature/NotFoundResponseTest.php new file mode 100644 index 00000000000..0a4101194c0 --- /dev/null +++ b/tests/Feature/NotFoundResponseTest.php @@ -0,0 +1,19 @@ +actingAs(User::factory()->create()) + ->getJson('/api/client/servers/deadbeef') + ->assertNotFound() + ->assertExactJson(['message' => 'This server could not be found. It may have been deleted.']); +}); + +it('names a missing node in plain words', function () { + $this->actingAs(User::factory()->create(['root_admin' => true])) + ->getJson('/api/admin/nodes/999999999') + ->assertNotFound() + ->assertJsonPath('message', 'This node could not be found. It may have been deleted.'); +}); diff --git a/tests/e2e/admin/not-found.spec.ts b/tests/e2e/admin/not-found.spec.ts new file mode 100644 index 00000000000..4823d951259 --- /dev/null +++ b/tests/e2e/admin/not-found.spec.ts @@ -0,0 +1,24 @@ +import { expect, test } from '@playwright/test' + +import { signInAsAdmin } from '../support/auth' + +test.beforeEach(async ({ page }) => { + await signInAsAdmin(page) +}) + +// Opening a deleted record used to show "Something went wrong" with Laravel's +// "No query results for model [...]" (servers), or crash outright (nodes, users). +for (const { path, record } of [ + { path: '/servers/deadbeef', record: 'Server' }, + { path: '/admin/servers/00000000-0000-4000-8000-00000000dead', record: 'Server' }, + { path: '/admin/nodes/999999999', record: 'Node' }, + { path: '/admin/users/999999999', record: 'User' }, +]) { + test(`says a missing ${record.toLowerCase()} is gone (${path})`, async ({ page }) => { + await page.goto(path) + + await expect(page.getByRole('heading', { name: `${record} not found` })).toBeVisible() + await expect(page.getByText('No query results')).toHaveCount(0) + await expect(page.getByRole('button', { name: 'Go Back' })).toBeVisible() + }) +} From 81b79d1334bd8c87b6b8b7e8605505f3ff74d5c5 Mon Sep 17 00:00:00 2001 From: Eric Wang <37554696+ericwang401@users.noreply.github.com> Date: Fri, 16 Oct 2026 00:45:59 +0000 Subject: [PATCH 2/2] fix: centre the not-found screen NotFound didn't pass ScreenBlock's center, so inside a page it sat at the left edge, unlike the other status screens. The full-page 404 is centred by its own layout and looks the same. --- resources/scripts/components/elements/ScreenBlock.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/resources/scripts/components/elements/ScreenBlock.tsx b/resources/scripts/components/elements/ScreenBlock.tsx index 728e4d7a409..b9aa6c7466f 100644 --- a/resources/scripts/components/elements/ScreenBlock.tsx +++ b/resources/scripts/components/elements/ScreenBlock.tsx @@ -107,6 +107,7 @@ export const NotFound = ({ "The link is either broken or doesn't exist on the server." } icon={LinkIcon} + center full={full} onBack={onBack} />