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

Feat/admin/webhooks #628

Merged
merged 17 commits into from
Nov 22, 2024
Merged

Conversation

ryanhopperlowe
Copy link
Contributor

@ryanhopperlowe ryanhopperlowe commented Nov 19, 2024

Screenshot 2024-11-20 at 11 14 18 AM Screenshot 2024-11-20 at 11 14 27 AM

@ryanhopperlowe
Copy link
Contributor Author

ryanhopperlowe commented Nov 20, 2024

The largest chunk of this PR is adding in the multi-select.tsx component (about 700 lines). That one can largely be skipped during review

@ryanhopperlowe ryanhopperlowe force-pushed the feat/admin/webhooks branch 2 times, most recently from 5a8fa84 to 2bcee12 Compare November 20, 2024 18:10
ui/admin/app/components/webhooks/WebhookConfirmation.tsx Outdated Show resolved Hide resolved
ui/admin/app/components/webhooks/WebhookForm.tsx Outdated Show resolved Hide resolved
control={form.control}
name="token"
label="Token (optional)"
description="Optionally provide a token to add an extra layer of security."
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should either add more information here or link to docs (if they exist) to explain what this does. Whatever token is set will have to be provided by the webhook provider (i.e. GitHub) in a particular query param or header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I mistakenly assumed the token would be provided in the returned link meta. I'll update this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I discussed this with Will and he decided to hide the token feature entirely until we can talk it out more with Bill/Donnie

ui/admin/app/components/webhooks/WebhookFormContext.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@StrongMonkey StrongMonkey left a comment

Choose a reason for hiding this comment

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

LGTM, just one small question

Copy link
Contributor

@thedadams thedadams left a comment

Choose a reason for hiding this comment

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

I noticed that the webhook create and edit pages take up the full-screen width. It looks a little weird on my larger monitor. Does it make sense to shrink the width down?

ui/admin/app/components/webhooks/WebhookForm.tsx Outdated Show resolved Hide resolved
@ryanhopperlowe ryanhopperlowe force-pushed the feat/admin/webhooks branch 3 times, most recently from 3ecf654 to ecd5786 Compare November 21, 2024 19:54
@thedadams
Copy link
Contributor

A few thoughts here:

  1. Let's remove alias until we get feedback from Craig on it.
  2. I am not able to click the "No Secret" checkbox. Also, not sure what it is used for. The user isn't required to enter a secret.
  3. * shouldn't be the default value for headers. Maybe add some detail text to explain that * can be used to include all headers.
Screenshot 2024-11-22 at 06 39 10

@ryanhopperlowe
Copy link
Contributor Author

A few thoughts here:

  1. Let's remove alias until we get feedback from Craig on it.
  2. I am not able to click the "No Secret" checkbox. Also, not sure what it is used for. The user isn't required to enter a secret.
  3. * shouldn't be the default value for headers. Maybe add some detail text to explain that * can be used to include all headers.
  1. removed
  2. Updated to render the checkbox conditionally in edit mode if the existing webhook already has a secret
  3. discussed offline

@ryanhopperlowe ryanhopperlowe merged commit 9d6a1bc into otto8-ai:main Nov 22, 2024
1 check passed
@ryanhopperlowe ryanhopperlowe deleted the feat/admin/webhooks branch November 22, 2024 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants