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

refactor: proxy callbacks #9

Merged

Conversation

Keyrxng
Copy link
Contributor

@Keyrxng Keyrxng commented Oct 23, 2024

Resolves #10
Required by ubiquity-os/plugins-wishlist#45

This PR adds the proxyCallback logic that we use in other plugins. I'm adding it because it's the cleanest most effective way to handle type-safe payloads across every webhook event and I need it for handling pull_request events.


I've noticed a separate issue regarding Logs which is that sometimes we get stack traces, sometimes we don't which is not good.

 if (!issue.body || !issue.html_url) {
    throw logger.error("Issue body or URL not found", { issueUrl: issue.html_url });
  }

This should generate a stack but instead we get:

image

  if (!issue.body || !issue.html_url) {
    throw logger.error("Issue body or URL not found", { issueUrl: issue.html_url, error: new Error("Issue body or URL not found") });
  }

If we manually create an error in the metadata we get a stack trace but you can clearly see that error is an empty object which is wrong which is where my suspicion comes from that the logger is wiping out that property under certain conditions.
image

Will open a task to get it addressed

Copy link
Contributor

github-actions bot commented Oct 23, 2024

Unused types (1)

Filename types
src/types/github-types.ts IssueWithUser

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Oct 23, 2024

QA: ubq-testing#7

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Oct 23, 2024

I'm unable to request review for some reason so gonna tag instead

@gentlementlegen @0x4007 @sshivaditya2019 @sshivaditya @rndquu

@Keyrxng Keyrxng mentioned this pull request Oct 23, 2024
@0x4007
Copy link
Member

0x4007 commented Oct 24, 2024

I've never got my own kernel/plugin set up so I don't have any context on this problem and can't add any value in the form of a review.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Oct 24, 2024

I've never got my own kernel/plugin set up so I don't have any context on this problem and can't add any value in the form of a review.

It's the solution to this ubiquity-os/ubiquity-os-kernel#10

src/main.ts Outdated Show resolved Hide resolved
@Keyrxng
Copy link
Contributor Author

Keyrxng commented Oct 25, 2024

QA: ubq-testing#7 (comment)

src/helpers/callback-proxy.ts Show resolved Hide resolved
src/handlers/comment-created-callback.ts Outdated Show resolved Hide resolved
.github/workflows/compute.yml Outdated Show resolved Hide resolved
@Keyrxng
Copy link
Contributor Author

Keyrxng commented Oct 28, 2024

QA: ubq-testing#11 (comment)

Good to merge? @sshivaditya2019 @gentlementlegen @whilefoo

@0x4007
Copy link
Member

0x4007 commented Oct 28, 2024

It's extremely redundant. This can all be expressed in a sentence or two.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Oct 28, 2024

It's extremely redundant. This can all be expressed in a sentence or two.

I don't understand what you are referring to

QA demonstrates that the plugin is working that's all it has nothing to do with the content in the LLM response. This PR is about adding the proxy so we can handle multiple webhook event payloads. If you are referring to ground truths there a spec to improve them #17

Copy link
Member

@whilefoo whilefoo left a comment

Choose a reason for hiding this comment

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

nice to see some code refactoring!

Copy link
Member

@gentlementlegen gentlementlegen left a comment

Choose a reason for hiding this comment

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

Don't hesitate to move that to the SDK, it makes things much simpler!

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Oct 29, 2024

Don't hesitate to move that to the SDK, it makes things much simpler!

Yeah we definitely should build our webhook handler logic into the SDK

I have a couple of tasks I'll close out first but I won't forget about it

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Oct 30, 2024

I can't merge if it's me we are waiting on for this fellas

@gentlementlegen gentlementlegen merged commit 8ea7b0f into ubiquity-os-marketplace:development Oct 30, 2024
2 checks passed
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.

Add proxy callback logic
4 participants