src: add and delete file webhooks when designs are attached or removed - #260
Conversation
| "test:unit": "DOTENV_CONFIG_PATH=.env.test jest -c jest.config.unit.ts" | ||
| }, | ||
| "dependencies": { | ||
| "@figma/rest-api-spec": "^0.33.0", |
There was a problem hiding this comment.
I replaced some of our custom types with types from our official rest api spec where possible
There was a problem hiding this comment.
praise: Amazing! Thank you for doing this, @Isaiah-Turner !
| DEV_MODE_STATUS_UPDATE = 'DEV_MODE_STATUS_UPDATE', | ||
| } | ||
|
|
||
| export class FigmaFileWebhook { |
There was a problem hiding this comment.
suggestion (npn-blocking): It seems we over-engineered a bit FigmaFileWebhook. This class is just holds data and does not have any business logic associated with it.
We could define FigmaFileWebhook as a read-only type without using classes.
export enum FigmaFileWebhookEventType {
FILE_UPDATE = 'FILE_UPDATE',
DEV_MODE_STATUS_UPDATE = 'DEV_MODE_STATUS_UPDATE',
}
export type FigmaFileWebhook = {
readonly id: string;
readonly webhookId: string;
readonly webhookPasscode: string;
readonly fileKey: string;
readonly eventType: FigmaFileWebhookEventType;
readonly createdBy: {
readonly atlassianUserId: string;
readonly connectInstallationId: string;
};
};
export type FigmaFileWebhookCreateParams = {
readonly webhookId: string;
readonly webhookPasscode: string;
readonly fileKey: string;
readonly eventType: FigmaFileWebhookEventType;
readonly createdBy: {
readonly atlassianUserId: string;
readonly connectInstallationId: string;
};
};There was a problem hiding this comment.
This is a great point! I fixed this in 6175d34
| }; | ||
|
|
||
| export const CREATE_WEBHOOK_RESPONSE: JSONSchemaTypeWithId<CreateWebhookResponse> = | ||
| export const CREATE_WEBHOOK_RESPONSE: JSONSchemaTypeWithId<PostWebhookResponse> = |
There was a problem hiding this comment.
nitpick Please, could you rename the schema to match the type name (CREATE_WEBHOOK_RESPONSE -> POST_WEBHOOK_RESPONSE) for consistency?
| readonly accessToken: string; | ||
| }; | ||
|
|
||
| export type CreateWebhookRequest = { |
There was a problem hiding this comment.
praise: This is so good!
| try { | ||
| const webhookPasscode = uuidv4(); | ||
| const { fileWebhook, devModeStatusUpdateWebhook } = | ||
| await figmaService.createFileContextWebhooks(fileKey, webhookPasscode, { |
There was a problem hiding this comment.
issue: The current behaviour can cause creating "dangling" webhooks in an edge case when the first webhook is successfully created but the creation of the second one fails. This being the case, no webhook will be recorded in our database. As a result, when the operation is retried by Atlassian's platform, the implementation will try to create a new set of webhooks.
In order to minimise the risk of this (it can still happens anyway if the database transaction fails), please, consider the following:
- Split
FigmaService.createFileContextWebhooksinto the methods:FigmaService.createFileUpdateWebHookandFigmaService.createDevModeStatusUpdateWebHook. - Update
maybeCreateFigmaFileWebhooksto accept an additional parameter of typeFigmaFileWebhookEventTypeand take responsibility of the a creation of a single webhook. It could do the following:- Check whether the webhook of the given type exists. If not -- return.
- Call
FigmaService.createFileContextWebhooks. - Call
FigmaFileWebhookRepository.upsertfor the created webhook.
- Call
maybeCreateFigmaFileWebhooksfor each supported webhook type (once forFILE_UPDATEandonce forDEV_MODE_STATUS_UPDATE).
What do you think, @Isaiah-Turner ?
There was a problem hiding this comment.
Yeah, this is a great call. I refactored this how you described in bf0acd7, and added a new test in src/web/routes/entities-v2/integration.test.ts
There was a problem hiding this comment.
suggestion (non-blocking): This use case is called on a PUT /entities/onEntityAssociated endpoint request. There are integration tests for this endpoint in src/web/routes/entities-v2/integration.test.ts, which are, likely, passing because the Launch Darkly client falls back to false for the feature gate.
I would highly recommend updating these tests in accordance with the new behaviour within this PR for extra safety.
Beware, that if we delay this work, we will not be able to clean up the feature gate without updating the affected tests.
This is totally fine to do this within a separate PR :)
|
praise: Great work, @Isaiah-Turner on the feature! And thank you so much for the detailed PR description and demos! |
akostevich-atlassian
left a comment
There was a problem hiding this comment.
Please, see other comments for more details.
f619d4b to
bf0acd7
Compare
bf0acd7 to
0b25a17
Compare
|
praise: This was a quite large and challenging work, @Isaiah-Turner ! Really love the changes and what they bring to the product. Great job! |
This PR adds logic to manage creating and deleting webhooks for a file when it is associated or disassociated with a design. It also adds a
FigmaFileRepositoryto make use of the new table from #259.On association, we now call
maybeCreateFigmaFileWebhooks. This function checks to see if the file already has file webhooks with the same file key. If not, we create aFILE_UPDATEandDEV_MODE_STATUS_UPDATEwebhook for that file.On disassociate, we call
maybeTryDeleteFigmaFileWebhooks. If there are no longer any linked designs that rely on the webhook, we'll delete them.Testing
Screen.Recording.2025-08-01.at.4.10.13.PM.mov
attaching-multiple-design.mov