Skip to content

improve(integrations/slack): sentry alert on error #6537

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
134 changes: 134 additions & 0 deletions packages/web/app/src/lib/fetch-json.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
import { z } from 'zod';
import { Kit } from './kit';

export async function fetchJson<schema extends undefined | z.ZodType = undefined>(
url: string,
requestInit?: RequestInit,
schema?: schema,
): Promise<
(schema extends z.ZodType ? z.infer<schema> : Kit.Json.Value) | FetchJsonErrors.FetchJsonErrors
> {
const response = await fetch(url, requestInit)
// @see https://developer.mozilla.org/en-US/docs/Web/API/Window/fetch#exceptions
.catch(Kit.oneOf(error => error instanceof TypeError || error instanceof DOMException));

if (response instanceof TypeError) {
return new FetchJsonErrors.FetchJsonRequestTypeError({ requestInit }, response);
}

if (response instanceof DOMException) {
return new FetchJsonErrors.FetchJsonRequestNetworkError({}, response);
}

const json = await response
.json()
.then(value => value as Kit.Json.Value)
// @see https://developer.mozilla.org/en-US/docs/Web/API/Response/json#exceptions
.catch(
Kit.oneOf(
error =>
error instanceof SyntaxError ||
error instanceof TypeError ||
error instanceof DOMException,
),
);

if (json instanceof DOMException) {
return new FetchJsonErrors.FetchJsonRequestNetworkError({}, json);
}

if (json instanceof TypeError) {
return new FetchJsonErrors.FetchJsonResponseTypeError({ response }, json);
}

if (json instanceof SyntaxError) {
return new FetchJsonErrors.FetchJsonResponseSyntaxError({ response }, json);
}

if (schema) {
const result = schema.safeParse(json);
if (!result.success) {
return new FetchJsonErrors.FetchJsonResponseSchemaError(
{ response, json, schema },
result.error,
);
}
return result.data as any; // z.infer<Exclude<schema, undefined>>;
}

return json as any; // Kit.Json.Value;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: prefer unknown to force us to use validation or cast

Copy link
Member Author

Choose a reason for hiding this comment

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

Since this is in the return position of an implementation it would not work and it does not matter, TS just doesn't make it possible to make implementations type safe. At best we could cast it to something matching the return type but its more maintenance for little gain and no increased type safety since it is a cast either way.

}

// =================================
// Error Classes
// =================================

// eslint-disable-next-line @typescript-eslint/no-namespace
export namespace FetchJsonErrors {
export type FetchJsonErrors = FetchJsonResponseErrors | FetchJsonRequestErrors;

// --------------------------------
// Response Error Classes
// --------------------------------

export type FetchJsonRequestErrors = FetchJsonRequestTypeError | FetchJsonRequestNetworkError;

export class FetchJsonRequestNetworkError extends Kit.Errors.ContextualError<
'FetchJsonRequestNetworkError',
{},
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid using {} as a type.

The lint rule recommends replacing {} with a more explicit shape or Record<string, never> to avoid implying any non-nullable value.

Proposed fix:

-export class FetchJsonRequestNetworkError extends Kit.Errors.ContextualError<
-  'FetchJsonRequestNetworkError',
-  {},
-  DOMException
- > {
+export class FetchJsonRequestNetworkError extends Kit.Errors.ContextualError<
+  'FetchJsonRequestNetworkError',
+  Record<string, never>,
+  DOMException
+> {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{},
export class FetchJsonRequestNetworkError extends Kit.Errors.ContextualError<
'FetchJsonRequestNetworkError',
Record<string, never>,
DOMException
> {
// ...rest of the class implementation
}
🧰 Tools
🪛 Biome (1.9.4)

[error] 78-78: Don't use '{}' as a type.

Prefer explicitly define the object shape. '{}' means "any non-nullable value".

(lint/complexity/noBannedTypes)

DOMException
> {
message = 'Network failure.';
}

export class FetchJsonRequestTypeError extends Kit.Errors.ContextualError<
'FetchJsonRequestTypeError',
{ requestInit?: RequestInit },
TypeError
> {
message = 'Invalid request.';
}

// --------------------------------
// Response Error Classes
// --------------------------------

export abstract class FetchJsonResponseError<
$Name extends string,
$Context extends {
response: Response;
},
$Cause extends z.ZodError | SyntaxError | TypeError | DOMException,
> extends Kit.Errors.ContextualError<$Name, $Context, $Cause> {
message = 'Invalid response.';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer if we can provide more insight than this. E.g. should we include the cause's error message?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's all available through the chain data in a structured way. The missing part IMO is error renderers that are up to date with modern error types. Cause is part of the ECMA standard so it's confusing to me that default renderings are still AFAIK failing to raise good context about them. We would just have to add our own where it is needed such as top level catch all or logging middleware, etc.

}

export type FetchJsonResponseErrors =
| FetchJsonResponseSyntaxError
| FetchJsonResponseSchemaError
| FetchJsonResponseTypeError;

export class FetchJsonResponseTypeError extends FetchJsonResponseError<
'FetchJsonResponseTypeError',
{ response: Response },
TypeError
> {
message = 'Response is malformed.';
}

export class FetchJsonResponseSyntaxError extends FetchJsonResponseError<
'FetchJsonResponseSyntaxError',
{ response: Response },
SyntaxError
> {
message = 'Response body is not valid JSON.';
}

export class FetchJsonResponseSchemaError extends FetchJsonResponseError<
'FetchJsonResponseSchemaError',
{ response: Response; json: Kit.Json.Value; schema: z.ZodType },
z.ZodError
> {
message = 'Response body JSON violates the schema.';
}
}
33 changes: 33 additions & 0 deletions packages/web/app/src/lib/kit/errors.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// eslint-disable-next-line @typescript-eslint/no-namespace
export namespace Errors {
export abstract class ContextualError<
$Name extends string = string,
$Context extends object = object,
$Cause extends Error | undefined = Error | undefined,
> extends Error {
public name: $Name;
public context: $Context;
public cause: $Cause;
constructor(
...args: undefined extends $Cause
? [context: $Context, cause?: $Cause]
: [context: $Context, cause: $Cause]
) {
const [context, cause] = args;
super('Something went wrong.', { cause });
this.name = this.constructor.name as $Name;
this.context = context;
this.cause = cause as $Cause;
Copy link
Collaborator

Choose a reason for hiding this comment

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

should the call to super set this?

}
}
Comment on lines +3 to +22
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider enhancing the error message handling.

The ContextualError class provides a good foundation for contextual errors, but the default error message "Something went wrong." is too generic.

Consider these improvements:

  1. Add an abstract method for generating error messages:
 export abstract class ContextualError<
   $Name extends string = string,
   $Context extends object = object,
   $Cause extends Error | undefined = Error | undefined,
 > extends Error {
   public name: $Name;
   public context: $Context;
   public cause: $Cause;
+  protected abstract formatMessage(): string;
   constructor(
     ...args: undefined extends $Cause
       ? [context: $Context, cause?: $Cause]
       : [context: $Context, cause: $Cause]
   ) {
     const [context, cause] = args;
-    super('Something went wrong.', { cause });
+    super('', { cause });
     this.name = this.constructor.name as $Name;
     this.context = context;
     this.cause = cause as $Cause;
+    this.message = this.formatMessage();
   }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export abstract class ContextualError<
$Name extends string = string,
$Context extends object = object,
$Cause extends Error | undefined = Error | undefined,
> extends Error {
public name: $Name;
public context: $Context;
public cause: $Cause;
constructor(
...args: undefined extends $Cause
? [context: $Context, cause?: $Cause]
: [context: $Context, cause: $Cause]
) {
const [context, cause] = args;
super('Something went wrong.', { cause });
this.name = this.constructor.name as $Name;
this.context = context;
this.cause = cause as $Cause;
}
}
export abstract class ContextualError<
$Name extends string = string,
$Context extends object = object,
$Cause extends Error | undefined = Error | undefined,
> extends Error {
public name: $Name;
public context: $Context;
public cause: $Cause;
+ protected abstract formatMessage(): string;
constructor(
...args: undefined extends $Cause
? [context: $Context, cause?: $Cause]
: [context: $Context, cause: $Cause]
) {
const [context, cause] = args;
- super('Something went wrong.', { cause });
+ super('', { cause });
this.name = this.constructor.name as $Name;
this.context = context;
this.cause = cause as $Cause;
+ this.message = this.formatMessage();
}
}


export class TypedAggregateError<$Error extends Error> extends AggregateError {
constructor(
public errors: $Error[],
message?: string,
) {
super(errors, message);
this.name = this.constructor.name;
}
}
}
17 changes: 17 additions & 0 deletions packages/web/app/src/lib/kit/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,20 @@ export const tryOr = <$PrimaryResult, $FallbackResult>(
return fallback();
}
};

export const oneOf = <type extends readonly unknown[]>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Please add a description block to make the behavior more obvious when we use this function.
When seeing it used, I wasnt 100% sure it threw, or what type of error it would throw.

...guards: OneOfCheck<type>
): ((value: unknown) => type[number]) => {
return (value: unknown) => {
for (const guard of guards) {
if (guard(value)) {
return value;
}
}
throw new Error(`Unexpected value received by oneOf: ${value}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be a unique error class?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe! Its failure represents a defect not a normal error so we could encode that in a special class for sure.

};
};

type OneOfCheck<types extends readonly unknown[]> = {
[index in keyof types]: (value: unknown) => value is types[index];
};
1 change: 1 addition & 0 deletions packages/web/app/src/lib/kit/index_.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ export * from './types/headers';
export * from './helpers';
export * from './json';
export * from './zod-helpers';
export * from './errors';
73 changes: 73 additions & 0 deletions packages/web/app/src/lib/slack-api.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import { stringify } from 'node:querystring';
import { z } from 'zod';
import { fetchJson } from './fetch-json';

// eslint-disable-next-line @typescript-eslint/no-namespace
export namespace SlackAPI {
Copy link
Collaborator

Choose a reason for hiding this comment

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

personally im not a fan of namespaces in typescript. I think namespacing by import path is good enough and this just adds an extra layer of complexity.
If typescript had a global namespace like ruby, then namespaces would serve a more important role of course.

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer using ESM but I received pushback using that approach on this repo. If the desired style of this repo is ultimately to use types and functions with stringly namespaced identifiers (e.g. slackAPIauthorize(...) instead of `SlackAPI.authorize(...)) then I wasn't aware of that, and I think the DX on that is worse. I usually trade producer complexity for consumer simplicity based on my feeling that complexity should go down, not up, the stack.

// ==================================
// Data Utilities
// ==================================

export const createOauth2AuthorizeUrl = (parameters: {
state: string;
clientId: string;
redirectUrl: string;
scopes: string[];
}) => {
const url = new URL('https://slack.com/oauth/v2/authorize');
const searchParams = new URLSearchParams({
scope: parameters.scopes.join(','),
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Slack scopes should be space-separated instead of comma-separated.

According to Slack's documentation, the scope parameter expects space-separated scopes rather than comma-separated. Using commas may cause request failures or unexpected behavior.

Apply this diff to fix the scopes parameter:

- scope: parameters.scopes.join(','),
+ scope: parameters.scopes.join(' '),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
scope: parameters.scopes.join(','),
scope: parameters.scopes.join(' '),

client_id: parameters.clientId,
redirect_uri: parameters.redirectUrl,
state: parameters.state,
});

url.search = searchParams.toString();
return url.toString();
};

// ==================================
// Request Methods
// ==================================

// ----------------------------------
// OAuth2AccessResult
// ----------------------------------

const OAuth2AccessResult = z.discriminatedUnion('ok', [
z.object({
ok: z.literal(true),
access_token: z.string(),
}),
z.object({
ok: z.literal(false),
error: z.string(),
}),
]);

export type OAuth2AccessResult = z.infer<typeof OAuth2AccessResult>;

export interface OAuth2AccessPayload {
clientId: string;
clientSecret: string;
code: string;
}

export async function requestOauth2Access(payload: OAuth2AccessPayload) {
return fetchJson(
'https://slack.com/api/oauth.v2.access',
{
method: 'POST',
headers: {
'content-type': 'application/x-www-form-urlencoded',
},
body: stringify({
client_id: payload.clientId,
client_secret: payload.clientSecret,
code: payload.code,
}),
},
OAuth2AccessResult,
);
}
}
Loading
Loading