diff --git a/CHANGELOG.md b/CHANGELOG.md
index 091d0e1b039..a35002d2b82 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -50,6 +50,10 @@ follows [Semantic Versioning](https://semver.org) guidelines.
- Fixed failing jobs retrying with no pause. A job whose request to Proxmox failed was retried at once, again and
again, until it ran out of time -- thousands of requests a minute against a node that was already struggling. Jobs
now wait a few seconds, then longer, between retries.
+- 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
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/components/elements/ScreenBlock.tsx b/resources/scripts/components/elements/ScreenBlock.tsx
index cc9e87b3201..84c58b3ce64 100644
--- a/resources/scripts/components/elements/ScreenBlock.tsx
+++ b/resources/scripts/components/elements/ScreenBlock.tsx
@@ -118,6 +118,7 @@ export const NotFound = ({
"The link is either broken or doesn't exist on the server."
}
icon={LinkIcon}
+ center
full={full}
onBack={onBack}
/>
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 aa2b77e7376..2e005be3923 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'
@@ -95,6 +98,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
@@ -118,8 +123,10 @@ const AdminServerRouter = () => {
useEffect(() => {
setError(undefined)
+ setNotFound(false)
getServer(match!.params.id as string).catch((error: any) => {
+ setNotFound(isNotFoundError(error))
setError(httpErrorToHuman(error))
})
@@ -143,7 +150,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 da948e80dbe..a380e54bee1 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'
@@ -130,6 +133,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
@@ -159,8 +164,10 @@ const ServerRouter = () => {
useEffect(() => {
setError(undefined)
+ setNotFound(false)
getServer(id as string).catch((error: any) => {
+ setNotFound(isNotFoundError(error))
setError(httpErrorToHuman(error))
})
@@ -261,7 +268,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()
+ })
+}