Conversation
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:3923 This environment will automatically shut down when the PR is closed or after 5 hours. |
guillim
left a comment
There was a problem hiding this comment.
LGTM, expect a need for synchronisation with our providers where we may have defined webhooks that have API key
| @Body() body: JsonRpc, | ||
| @AuthWorkspace() workspace: WorkspaceEntity, | ||
| @AuthApiKey() apiKey: string | undefined, | ||
| @AuthApiKey() apiKey: ApiKeyEntity | undefined, |
There was a problem hiding this comment.
this is going to break any current MCP users right ? are we ok with that ?
There was a problem hiding this comment.
This is only a typing change, this won't break the actual logic behind it. Also, request object already types it correctly but this was not typed properly in controllers/resolvers.
The typing was wrong originally already, code expected things to be strings but they were objects. They were rarely accessed as values but simply checked if they were defined but I'd prefer to correct the typing right now to avoid any issue in the future. We had a real bug that I've fixed in this PR if you want an example #16540
There was a problem hiding this comment.
My bad, I did not read 16540. Then it's 100% logic
Following this #16540
Those were not failing (yet) but were wrongly typed and error prone