Wrap server actions in sentry HOF; add custom global-error files by 3mcd · Pull Request #306 · pubpub/platform

3mcd · 2024-04-04T11:25:42Z

See the server actions explainer for a description of the server actions-related changes.

This PR:

Updates Sentry to fix a very bizarre issue with only certain server actions.
Converts deprecated new Sentry.ReplayIntegration to Sentry.replayIntegration()
Adds root global-error.tsx files to core and integration Next apps. This component should forward all unhandled, non- server action application errors to Sentry.
Introduces defineServerAction, which wraps server actions with Sentry.withServerActionInstrumentation to properly forward uncaught errors in server actions to Sentry. Note, as of now, Sentry's higher-order-function isn't doing much since we're try/catching the result of the server action ourselves.
Provides a pattern for caught errors in server actions to be forwarded to Sentry with ClientException...
Adds a new interface called ClientException, which are exceptions sent from server actions to the client when something goes wrong in a server action. These objects should help us standardize how we structure error responses and render them on the client.
Refactors existing destructive/error toasts to use the newly added ClientException interface via a new hook called useServerAction. This hook renders the ClientException error message and sentry id (if present).
Adds the Sentry error id to ClientException instances where possible, and renders the error id in the exception's subsequent toast.

Issue(s) Resolved

Resolves #299

Test Plan

Checkout the branch and add some predictable errors in a few server action bodies, and in a couple of routes/pages.
Comment out the if (env.NODE_ENV === "production") check in the sentry.*.config.ts files.
Start the app.
Trigger the errors.
- For server actions, you should see a destructive (red) toast when an error occurs. Depending on the server action, you may also see a Sentry error id that looks like the screenshot below.
- For pages/routes, you should see the default Next error screen.
- For both, you should see the errors reported in Sentry. Be sure to resolve/close your test errors in Sentry.

Screenshots (if applicable)

3mcd · 2024-04-04T15:11:56Z

core/app/c/[communitySlug]/members/[[...add]]/actions.ts

+	return withServerActionInstrumentation(
+		"members/revalidateMemberPathsAndTags",
+		{
+			headers: headers(),


Annoyingly, Sentry's withServerActionInstrumentation accepts a formData object (that must conform to the FormData API) but not an arbitrary payload. This option is completely useless to us since most of our server actions accept primitive/POJO parameters.

I did forward Next's headers along for some additional context, but that's about the best I could do.

Not that we actually need to add anything in there right now, but is there a reason we couldn't just construct a formData object? like:

const formData = new FormData(); for (const [k, v] of arbitraryPayload.values()) { formData.append(k, v) }

That would work, but since we pass multiple parameters and not just single objects with many keys, it could get tricky/annoying to have to name each one.

integrations/evaluations/app/actions/respond/actions.ts

…entation

tefkah · 2024-04-09T08:20:39Z

integrations/evaluations/app/actions/manage/actions.ts

i'll mark the files here going forward to ask about why not use defineServerAction in these places. if there is a good reason, no problem, but i think if we are going to introduce this pattern (which i think is good!) we implement it all in this PR unless we shouldn't for some reason

if the reason is "these are integrations and i don't care that much" that's a valid enough reason haha

Nevermind, the reason is "it's only defined in core", carry on

Yeah, a little bit of both!

tefkah · 2024-04-09T08:22:10Z

integrations/evaluations/app/global-error.tsx

nice, this makes me feel somewhat more confident that we won't miss any errors!

tefkah · 2024-04-09T08:44:08Z

core/lib/serverActions.md

thanks for including specific documentation! very helpful

…evel

tefkah · 2024-04-09T09:33:27Z

config/eslint/next.js

+		"no-restricted-syntax": [
+			"error",
+			{
+				selector:
+					"CallExpression[callee.name='defineServerAction'] > :nth-child(1):not(FunctionExpression[id.name][async=true])",
+				message: "You can only pass named, async functions into defineServerAction",
+			},
+		],


i added the eslint rule to this PR instead of doing a follow up, seems easier

See here: https://eslint.org/play/#eyJ0ZXh0IjoiLyogZXNsaW50IG5vLXJlc3RyaWN0ZWQtc3ludGF4OiBbXCJlcnJvclwiLCB7IFwic2VsZWN0b3JcIjogXCJDYWxsRXhwcmVzc2lvbltjYWxsZWUubmFtZT0nZGVmaW5lU2VydmVyQWN0aW9uJ10gPiA6bnRoLWNoaWxkKDEpOm5vdChGdW5jdGlvbkV4cHJlc3Npb25baWQubmFtZV1bYXN5bmM9dHJ1ZV0pXCIsIFwibWVzc2FnZVwiOiBcIllvdSBjYW4gb25seSBwYXNzIG5hbWVkLCBhc3luYyBmdW5jdGlvbnMgaW50byBkZWZpbmVTZXJ2ZXJBY3Rpb25cIn1dICovXG5cbmNvbnN0IGNyZWF0ZVN0YWdlID0gZGVmaW5lU2VydmVyQWN0aW9uKGFzeW5jIChjb21tdW5pdHlJZCk9PiB7XG5cdHRyeSB7XG5cdFx0YXdhaXQgZGIuc3RhZ2UuY3JlYXRlKHtcblx0XHRcdGRhdGE6IHtcblx0XHRcdFx0bmFtZTogXCJVbnRpdGxlZCBTdGFnZVwiLFxuXHRcdFx0XHRvcmRlcjogXCJhYVwiLFxuXHRcdFx0XHRjb21tdW5pdHk6IHtcblx0XHRcdFx0XHRjb25uZWN0OiB7XG5cdFx0XHRcdFx0XHRpZDogY29tbXVuaXR5SWQsXG5cdFx0XHRcdFx0fSxcblx0XHRcdFx0fSxcblx0XHRcdH0sXG5cdFx0fSk7XG5cdH0gY2F0Y2ggKGVycm9yKSB7XG5cdFx0cmV0dXJuIG1ha2VDbGllbnRFeGNlcHRpb24oXCJGYWlsZWQgdG8gY3JlYXRlIHN0YWdlXCIsIGNhcHR1cmVFeGNlcHRpb24oZXJyb3IpKTtcblx0fSBmaW5hbGx5IHtcblx0XHRyZXZhbGlkYXRlVGFnKGBjb21tdW5pdHktc3RhZ2VzXyR7Y29tbXVuaXR5SWR9YCk7XG5cdH1cbn0pOyBcblxuY29uc3QgY3JlYXRlU3RhZ2UyID0gZGVmaW5lU2VydmVyQWN0aW9uKGFzeW5jIGZ1bmN0aW9uIGNyZWF0ZVN0YWdlKGNvbW11bml0eUlkKSB7XG5cdHRyeSB7XG5cdFx0YXdhaXQgZGIuc3RhZ2UuY3JlYXRlKHtcblx0XHRcdGRhdGE6IHtcblx0XHRcdFx0bmFtZTogXCJVbnRpdGxlZCBTdGFnZVwiLFxuXHRcdFx0XHRvcmRlcjogXCJhYVwiLFxuXHRcdFx0XHRjb21tdW5pdHk6IHtcblx0XHRcdFx0XHRjb25uZWN0OiB7XG5cdFx0XHRcdFx0XHRpZDogY29tbXVuaXR5SWQsXG5cdFx0XHRcdFx0fSxcblx0XHRcdFx0fSxcblx0XHRcdH0sXG5cdFx0fSk7XG5cdH0gY2F0Y2ggKGVycm9yKSB7XG5cdFx0cmV0dXJuIG1ha2VDbGllbnRFeGNlcHRpb24oXCJGYWlsZWQgdG8gY3JlYXRlIHN0YWdlXCIsIGNhcHR1cmVFeGNlcHRpb24oZXJyb3IpKTtcblx0fSBmaW5hbGx5IHtcblx0XHRyZXZhbGlkYXRlVGFnKGBjb21tdW5pdHktc3RhZ2VzXyR7Y29tbXVuaXR5SWR9YCk7XG5cdH1cbn0pO1xuXG5jb25zdCBjcmVhdGVTdGFnZTMgPSBkZWZpbmVTZXJ2ZXJBY3Rpb24oYXN5bmMgZnVuY3Rpb24oY29tbXVuaXR5SWQpIHtcblx0dHJ5IHtcblx0XHRhd2FpdCBkYi5zdGFnZS5jcmVhdGUoe1xuXHRcdFx0ZGF0YToge1xuXHRcdFx0XHRuYW1lOiBcIlVudGl0bGVkIFN0YWdlXCIsXG5cdFx0XHRcdG9yZGVyOiBcImFhXCIsXG5cdFx0XHRcdGNvbW11bml0eToge1xuXHRcdFx0XHRcdGNvbm5lY3Q6IHtcblx0XHRcdFx0XHRcdGlkOiBjb21tdW5pdHlJZCxcblx0XHRcdFx0XHR9LFxuXHRcdFx0XHR9LFxuXHRcdFx0fSxcblx0XHR9KTtcblx0fSBjYXRjaCAoZXJyb3IpIHtcblx0XHRyZXR1cm4gbWFrZUNsaWVudEV4Y2VwdGlvbihcIkZhaWxlZCB0byBjcmVhdGUgc3RhZ2VcIiwgY2FwdHVyZUV4Y2VwdGlvbihlcnJvcikpO1xuXHR9IGZpbmFsbHkge1xuXHRcdHJldmFsaWRhdGVUYWcoYGNvbW11bml0eS1zdGFnZXNfJHtjb21tdW5pdHlJZH1gKTtcblx0fVxufSk7XG5cbmNvbnN0IGNyZWF0ZVN0YWdlNCA9IGRlZmluZVNlcnZlckFjdGlvbihmdW5jdGlvbihjb21tdW5pdHlJZCkge1xuXHR0cnkge1xuXHRcdGRiLnN0YWdlLmNyZWF0ZSh7XG5cdFx0XHRkYXRhOiB7XG5cdFx0XHRcdG5hbWU6IFwiVW50aXRsZWQgU3RhZ2VcIixcblx0XHRcdFx0b3JkZXI6IFwiYWFcIixcblx0XHRcdFx0Y29tbXVuaXR5OiB7XG5cdFx0XHRcdFx0Y29ubmVjdDoge1xuXHRcdFx0XHRcdFx0aWQ6IGNvbW11bml0eUlkLFxuXHRcdFx0XHRcdH0sXG5cdFx0XHRcdH0sXG5cdFx0XHR9LFxuXHRcdH0pO1xuXHR9IGNhdGNoIChlcnJvcikge1xuXHRcdHJldHVybiBtYWtlQ2xpZW50RXhjZXB0aW9uKFwiRmFpbGVkIHRvIGNyZWF0ZSBzdGFnZVwiLCBjYXB0dXJlRXhjZXB0aW9uKGVycm9yKSk7XG5cdH0gZmluYWxseSB7XG5cdFx0cmV2YWxpZGF0ZVRhZyhgY29tbXVuaXR5LXN0YWdlc18ke2NvbW11bml0eUlkfWApO1xuXHR9XG59KTtcblxuLy8gd2UgbGV0IHR5cGVzY3JpcHQgaGFuZGxlIHRoaXNcbmNvbnN0IGNyZWF0ZVN0YWdlNSA9IGRlZmluZVNlcnZlckFjdGlvbihjcmVhdGVTdGFnZTIpIiwib3B0aW9ucyI6eyJydWxlcyI6e30sImxhbmd1YWdlT3B0aW9ucyI6eyJzb3VyY2VUeXBlIjoic2NyaXB0IiwicGFyc2VyT3B0aW9ucyI6eyJlY21hRmVhdHVyZXMiOnt9fX19fQ==

(i also resolved the merge conflict that this created)

tefkah · 2024-04-09T09:37:29Z

core/lib/server/defineServerAction.ts

+ * @param serverActionFn
+ * @returns
+ */
+export const defineServerAction = <T extends (...args: unknown[]) => Promise<unknown>>(


I changed this into Promise<unknown> instead of unknown, to enforce the async-ness of the passed function on the type level as well. (ofc you can pass a non-async-promise-returning function as well, but it's at least a bit closer)

tefkah · 2024-04-09T09:39:44Z

core/lib/serverActions.ts

+export function useServerAction<T extends unknown[], U>(action: (...args: T) => Promise<U>) {
+	const runServerAction = useCallback(
+		async function runServerAction(...args: T) {
+			const result = await action(...args);
+			if (isClientException(result)) {
+				toast({
+					title: result.title ?? "Error",
+					variant: "destructive",
+					description: `${result.error}${result.id ? ` (Error ID: ${result.id})` : ""}`,
+				});
+			}
+			return result;
+		},
+		[action, toast]
+	);
+	return runServerAction;
+}
+


tefkah · 2024-04-09T10:07:47Z

Seems to work perfectly!

I tested this by manually removing a stage in the DB and then trying to remove it in the editor UI, which generated this Sentry error (https://kfg.sentry.io/issues/5162958068/?referrer=alert_email&alert_type=email&alert_timestamp=1712656911734&alert_rule_id=14700197&notification_uuid=318664ac-f227-408e-af8f-e0c666a07456&environment=development) and showed a toast as intended.

Might be a bit hard to test every single server action, but I have faith that this is working as it should!

Page errors are also properly sent to sentry

Both were tested by just putting a throw new Error somewhere in the body of the page in a client and server component respectively

3mcd marked this pull request as ready for review April 4, 2024 15:05

3mcd requested review from kalilsn, qweliant and tefkah April 4, 2024 15:05

3mcd commented Apr 4, 2024

View reviewed changes

tefkah reviewed Apr 4, 2024

View reviewed changes

integrations/evaluations/app/actions/respond/actions.ts Outdated Show resolved Hide resolved

3mcd added 9 commits April 4, 2024 16:38

Wrap server actions in sentry HOF; add custom global-error files

a8e88d9

Put global-error.tsx files in the correct place

1d6e7f5

wip

c21ccaf

client exception error

b615662

Use isClientException instead of truthiness check

80f479f

redirect outside of try/catch block

b7f18c2

Sentry upgrade; further abstract client exceptions and sentry instrum…

965c490

…entation

Quick docs

ac93b36

Format and lint

67441b6

3mcd force-pushed the em/sentry-improvements branch from 62f50b5 to 67441b6 Compare April 4, 2024 20:52

tefkah reviewed Apr 9, 2024

View reviewed changes

integrations/evaluations/app/global-error.tsx

Copy link

Member

tefkah Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, this makes me feel somewhat more confident that we won't miss any errors!

tefkah reviewed Apr 9, 2024

View reviewed changes

core/lib/serverActions.md

Copy link

Member

tefkah Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for including specific documentation! very helpful

tefkah added 2 commits April 9, 2024 11:26

feat: add eslint rule enforcing proper usage of defineServerAction

fc6e54e

feat: make server actions only accept promise functions on the type l…

0898788

…evel

tefkah reviewed Apr 9, 2024

View reviewed changes

tefkah approved these changes Apr 9, 2024

View reviewed changes

Merge branch 'main' into em/sentry-improvements

8148313

3mcd merged commit c204ca2 into main Apr 9, 2024

3mcd deleted the em/sentry-improvements branch April 9, 2024 12:28

Conversation

3mcd commented Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue(s) Resolved

Test Plan

Screenshots (if applicable)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kalilsn Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tefkah commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

3mcd commented Apr 4, 2024 •

edited

Loading

kalilsn Apr 4, 2024 •

edited

Loading

tefkah commented Apr 9, 2024 •

edited

Loading