Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved dashboard error msgs when credentials missing #113

Merged
merged 2 commits into from
Nov 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/root.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
export const loader = ({ context, request }: LoaderFunctionArgs) => {
const url = new URL(request.url);
return json({
version: context.cloudflare.env.CF_PAGES_COMMIT_SHA,
version: context.cloudflare?.env?.CF_PAGES_COMMIT_SHA,

Check warning on line 25 in app/root.tsx

View check run for this annotation

Codecov / codecov/patch

app/root.tsx#L25

Added line #L25 was not covered by tests
origin: url.origin,
url: request.url,
});
Expand Down
72 changes: 50 additions & 22 deletions app/routes/__tests__/dashboard.test.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// @vitest-environment jsdom
import { json } from "@remix-run/node";
import { json, LoaderFunctionArgs } from "@remix-run/node";
import {
vi,
test,
Expand Down Expand Up @@ -36,29 +36,57 @@ describe("Dashboard route", () => {
});

describe("loader", () => {
test("throws an exception if no Cloudflare credentials are provided", async () => {
// empty strings
await expect(
loader({
context: {
analyticsEngine: new AnalyticsEngineAPI(
"testAccountId",
"testApiToken",
),
cloudflare: {
// @ts-expect-error we don't need to provide all the properties of the cloudflare object
env: {
CF_BEARER_TOKEN: "",
CF_ACCOUNT_ID: "",
},
test("throws a 501 Response if no Cloudflare credentials are provided", async () => {
const mockLoaderParams: LoaderFunctionArgs = {
context: {
analyticsEngine: new AnalyticsEngineAPI(
"testAccountId",
"testApiToken",
),
cloudflare: {
// @ts-expect-error we don't need to provide all the properties of the cloudflare object
env: {
CF_ACCOUNT_ID: "",
CF_BEARER_TOKEN: "",
},
},
// @ts-expect-error we don't need to provide all the properties of the request object
request: {
url: "http://localhost:3000/dashboard",
},
}),
).rejects.toThrow("Missing Cloudflare credentials");
},
// @ts-expect-error we don't need to provide all the properties of the request object
request: {
url: "http://localhost:3000/dashboard",
},
};

try {
await loader(mockLoaderParams);
} catch (error) {
expect(error).toBeInstanceOf(Response);
const response = error as Response;
expect(await response.text()).toBe(
"Missing credentials: CF_ACCOUNT_ID is not set.",
);
expect(response.status).toBe(501);
}

// run it again, this time with account ID present, but bearer token absent
mockLoaderParams.context.cloudflare = {
// @ts-expect-error we don't need to provide all the properties of the cloudflare object
env: {
CF_ACCOUNT_ID: "testAccountId",
CF_BEARER_TOKEN: "",
},
};

try {
await loader(mockLoaderParams);
} catch (error) {
expect(error).toBeInstanceOf(Response);
const response = error as Response;
expect(await response.text()).toBe(
"Missing credentials: CF_BEARER_TOKEN is not set.",
);
expect(response.status).toBe(501);
}
});

test("redirects to ?site=siteId if no siteId is provided via query string", async () => {
Expand Down
35 changes: 29 additions & 6 deletions app/routes/dashboard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@
import type { LoaderFunctionArgs, MetaFunction } from "@remix-run/cloudflare";
import { json, redirect } from "@remix-run/cloudflare";
import {
isRouteErrorResponse,
useLoaderData,
useNavigation,
useRouteError,
useSearchParams,
} from "@remix-run/react";

Expand Down Expand Up @@ -41,13 +43,16 @@

export const loader = async ({ context, request }: LoaderFunctionArgs) => {
// NOTE: probably duped from getLoadContext / need to de-duplicate
if (
!context.cloudflare.env.CF_BEARER_TOKEN ||
!context.cloudflare.env.CF_ACCOUNT_ID
) {
throw new Error("Missing Cloudflare credentials");
if (!context.cloudflare?.env?.CF_ACCOUNT_ID) {
throw new Response("Missing credentials: CF_ACCOUNT_ID is not set.", {
status: 501,
});
}
if (!context.cloudflare?.env?.CF_BEARER_TOKEN) {
throw new Response("Missing credentials: CF_BEARER_TOKEN is not set.", {
status: 501,
});
}

const { analyticsEngine } = context;

const url = new URL(request.url);
Expand Down Expand Up @@ -273,3 +278,21 @@
</div>
);
}

export function ErrorBoundary() {
const error = useRouteError();

Check warning on line 283 in app/routes/dashboard.tsx

View check run for this annotation

Codecov / codecov/patch

app/routes/dashboard.tsx#L283

Added line #L283 was not covered by tests

const errorTitle = isRouteErrorResponse(error) ? error.status : "Error";
const errorBody = isRouteErrorResponse(error)
? error.data
: error instanceof Error
? error.message
: "Unknown error";

Check warning on line 290 in app/routes/dashboard.tsx

View check run for this annotation

Codecov / codecov/patch

app/routes/dashboard.tsx#L285-L290

Added lines #L285 - L290 were not covered by tests

return (
<div className="border-2 rounded p-4 bg-card">
<h1 className="text-2xl font-bold">{errorTitle}</h1>
<p className="text-lg">{errorBody}</p>
</div>

Check warning on line 296 in app/routes/dashboard.tsx

View check run for this annotation

Codecov / codecov/patch

app/routes/dashboard.tsx#L292-L296

Added lines #L292 - L296 were not covered by tests
);
}

Check warning on line 298 in app/routes/dashboard.tsx

View check run for this annotation

Codecov / codecov/patch

app/routes/dashboard.tsx#L298

Added line #L298 was not covered by tests
7 changes: 0 additions & 7 deletions load-context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,6 @@ type GetLoadContext = (args: {

// Shared implementation compatible with Vite, Wrangler, and Cloudflare Pages
export const getLoadContext: GetLoadContext = ({ context }) => {
if (
!context.cloudflare.env.CF_BEARER_TOKEN ||
!context.cloudflare.env.CF_ACCOUNT_ID
) {
throw new Error("Missing Cloudflare credentials");
}

const analyticsEngine = new AnalyticsEngineAPI(
context.cloudflare.env.CF_ACCOUNT_ID,
context.cloudflare.env.CF_BEARER_TOKEN,
Expand Down
6 changes: 3 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@
"@types/ua-parser-js": "^0.7.39",
"@typescript-eslint/eslint-plugin": "^7.13.1",
"@typescript-eslint/parser": "^7.13.1",
"@vitest/coverage-istanbul": "^1.6.0",
"@vitest/coverage-v8": "^1.6.0",
"@vitest/coverage-istanbul": "2.1",
"@vitest/coverage-v8": "2.1",
"autoprefixer": "^10.4.19",
"eslint": "^8.57.0",
"eslint-config-prettier": "^9.1.0",
Expand All @@ -71,7 +71,7 @@
"typescript": "^5.4.5",
"vite": "^5.3.1",
"vite-tsconfig-paths": "^4.3.2",
"vitest": "^1.6.0",
"vitest": "2.1",
"vitest-dom": "^0.1.1",
"wrangler": "^3.61.0"
},
Expand Down
Loading