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

fix: Post a highlight #101

Merged
merged 26 commits into from
May 29, 2023
Merged

fix: Post a highlight #101

merged 26 commits into from
May 29, 2023

Conversation

a0m0rajab
Copy link
Contributor

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • πŸ§‘β€πŸ’» Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert

Description

This adds "post on highlights" button to the extension for users.
fixes #88

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings

Post page
main paoge

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help

Added to documentation?

  • πŸ“œ README.md
  • πŸ““ docs.opensauced.pizza
  • πŸ• dev.to/opensauced
  • πŸ“• storybook
  • πŸ™… no documentation needed

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

@a0m0rajab a0m0rajab changed the title Fix: Post a highlight fix: Post a highlight May 22, 2023
Copy link
Member

@bdougie bdougie left a comment

Choose a reason for hiding this comment

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

This user experience is not great. We should change this to the following.

  1. The social card is not needed since the user is already on GitHub.
  2. We should populate the title (using the existing titles from the PR) and description (using the pr description generation endpoint)

On submit should use the AI.

@a0m0rajab
Copy link
Contributor Author

Sure, I will remove the social cards, as for the generation, I will be waiting an answer from anush or divyansh to implement it.

@Anush008
Copy link
Member

Anush008 commented May 23, 2023

Sure, I will remove the social cards, as for the generation, I will be waiting an answer from anush or divyansh to implement it.

We can conditionally render the "Post On Highlights" button in the extension drop-down page when the user visits a GitHub PR page authored by them and can have the title, PR URL pre-filled in the highlight creation form with a button to generate the description. I can add these once the we are good to go with your highlight creation form.

@a0m0rajab
Copy link
Contributor Author

Thank you anush, I was stuck at the URL thingy, but unblocked right now.

@bdougie
Copy link
Member

bdougie commented May 23, 2023

GitHub PR page authored by them

@Anush008 A user should be able to post highlights not authored by them. i.e. a feature they opened an issue on will be an option. Also, a maintainer or co-author is another scenario.

@Anush008
Copy link
Member

GitHub PR page authored by them

@Anush008 A user should be able to post highlights not authored by them. i.e. a feature they opened an issue on will be an option. Also, a maintainer or co-author is another scenario.

We have a check right now that prevents the "Add to highlights" button from being added to PRs not authored by the signed-in user. We'll have to remove that too.

@Anush008
Copy link
Member

Anush008 commented May 24, 2023

Hey @a0m0rajab. About 33eb47d, the message passing between content-scripts and the extension is prone to closed-receiving-end errors. Be sure to check for those on the chrome://extensions page.

@a0m0rajab
Copy link
Contributor Author

Thank you for that, I did not finish it yet, just committed to record my process and not lose it.

@a0m0rajab a0m0rajab marked this pull request as draft May 24, 2023 06:48
@a0m0rajab
Copy link
Contributor Author

Daily update: added the endpoint and pushed the highlight to the website, next steps:

  • disable the post button while sending the network request
  • show a toast/page for a successful post of highlight and add a link to it like the LinkedIn logic:
    linkedin UX
  • I will need to refactor the code as well before the last push.
  • an issue I faced while doing this is that I did not get the id of the highlight from the backend.

@a0m0rajab
Copy link
Contributor Author

Used statues code for error showing since there is no statue text right now.
image

@a0m0rajab
Copy link
Contributor Author

Here is a video of the extension:

2023-05-25_00-56-44.mp4

Is there feedback about it right now? In the next iteration, I will add the needed checks for the post to insights button to conditionally disable it. Why not render it? Cause I think if the user sees the button will get curios about how to use it, which will be explained in the button text/

@a0m0rajab
Copy link
Contributor Author

a0m0rajab commented May 24, 2023

As for the

pr description generation endpoint

I think it's not enabled right now, will have to check that later.

@a0m0rajab a0m0rajab marked this pull request as ready for review May 24, 2023 22:16
@a0m0rajab a0m0rajab mentioned this pull request May 25, 2023
2 tasks
@a0m0rajab
Copy link
Contributor Author

@Anush008 I got stuck with the AI endpoint logic.
Could we pairprogram it together? I did not know how to debug it.

I tried to follow a logic similar to the one you used, but it did not give me any output.

@Anush008
Copy link
Member

@Anush008 I got stuck with the AI endpoint logic. Could we pairprogram it together? I did not know how to debug it.

I tried to follow a logic similar to the one you used, but it did not give me any output.

Definitely. Hit me up on Discord when you're ready. @anush#5038..

@diivi
Copy link
Contributor

diivi commented May 25, 2023

Is there feedback about it right now?

Looks good, but I think it feels a little bland rn, maybe use opensauced orange for the submit button.

@bdougie
Copy link
Member

bdougie commented May 25, 2023

This will need a rebase from beta for a lint and merge conflict fix.

@a0m0rajab a0m0rajab requested a review from diivi May 27, 2023 17:51
@a0m0rajab
Copy link
Contributor Author

Side note: an option to have multiple shot prompts or even change the prompt to make it more feasible for social media would be great.

@bdougie
Copy link
Member

bdougie commented May 27, 2023

From my perspective I feel to merge this PR and work on issues on a new one, since there is a breaking PR coming in the next few days. What do you think?

Making this a minimally viable product is the goal.

  • Adding a generate button would be more ideal than automating it. Running it automatically cost money and might not be the experience a user expects.
  • I did not see animation or a title propagated when I ran this locally.

ezgif com-video-to-gif (5)

@diivi
Copy link
Contributor

diivi commented May 27, 2023

image

are these fixable?

@a0m0rajab
Copy link
Contributor Author

@bdougie are you on the latest commit? It's working locally for me. One thing that might need to be done is to refresh the page (CTRL+SHIFT+R) hard since the connection between content-scripts and popups might not work. If the issue is still after hard refresh can you share the output from the console in the extension and the GitHub page?

@diivi nope, this is a warning it keeps generating a warning when you solve it. There is a conflict in the lint options there.

2023-05-27_22-38-07.mp4

Copy link
Member

@bdougie bdougie left a comment

Choose a reason for hiding this comment

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

The copy needs updates to give the user more context

src/pages/posthighlight.tsx Outdated Show resolved Hide resolved
src/pages/posthighlight.tsx Outdated Show resolved Hide resolved
src/pages/posthighlight.tsx Outdated Show resolved Hide resolved
src/pages/posthighlight.tsx Outdated Show resolved Hide resolved
@bdougie
Copy link
Member

bdougie commented May 29, 2023

@a0m0rajab I updated all the copy to be more descriptive for the user. @diivi or @Anush008 can you give this a code review?

We can iterate on this after this is merged, but for basic functionaly, it works.

Screen Shot 2023-05-29 at 8 10 08 AM

@Anush008
Copy link
Member

Anush008 commented May 29, 2023

This PR uses message passing between the GH content-script and the pop-up page to get info from the GH webpage. Which as we previously experienced is prone to closed-receiving-end errors.
I do have an approach in mind to make this feature work without message passing. I can try it once we merge this. Other than that, this looks great. @a0m0rajab πŸ‘πŸ‘.

Copy link
Member

@Anush008 Anush008 left a comment

Choose a reason for hiding this comment

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

πŸš€

src/pages/posthighlight.tsx Show resolved Hide resolved
@bdougie
Copy link
Member

bdougie commented May 29, 2023

Let's merge this now as is and continue to iterate and improve this.

This will definitely need usage docs in docs.opensauced.pizza

@bdougie bdougie merged commit e3b7051 into open-sauced:beta May 29, 2023
@a0m0rajab
Copy link
Contributor Author

@bdougie thank you for that.
@Anush008 As for the prone, did you face this issue on this PR? and what approach you got on mind?
I got such an error but solved it by following the documentation mentioning the async functions:

In the above example, sendResponse() was called synchronously. If you want to asynchronously use sendResponse(), add return true; to the onMessage event handler.

which I used at this line:
https://github.com/open-sauced/ai/pull/101/files#diff-12e3b206dc980349f2c064d6bb1e0068655e929b8a4bf1ad88bf4fb06c15d0a0R79

github-actions bot pushed a commit that referenced this pull request May 29, 2023
## [1.3.1-beta.4](v1.3.1-beta.3...v1.3.1-beta.4) (2023-05-29)

### πŸ› Bug Fixes

* Post a highlight ([#101](#101)) ([e3b7051](e3b7051))
@github-actions
Copy link

πŸŽ‰ This PR is included in version 1.3.1-beta.4 πŸŽ‰

The release is available on GitHub release

Your semantic-release bot πŸ“¦πŸš€

@Anush008
Copy link
Member

Sometimes the message listener in the content-script unmounts and the sender(pop-up page) gets the closed-receiving-end errors. You can see the error log on the chrome://extensions page. Which is not a problem with the code but just a runtime issue.

@Anush008
Copy link
Member

Anush008 commented May 29, 2023

Our previous encounter with closed-receiving-end errors was resolved here. #64 (comment). 51a5a4e.

github-actions bot pushed a commit that referenced this pull request Jun 3, 2023
## [1.4.0](v1.3.0...v1.4.0) (2023-06-03)

### βœ… Tests

* add tests for checkAuth.ts ([#149](#149)) ([3a79fac](3a79fac))
* add tests for utils/urlMatchers.ts ([#143](#143)) ([26702e6](26702e6))

### πŸ› Bug Fixes

* Post a highlight ([#101](#101)) ([e3b7051](e3b7051))
* remove auto-take message from triage.yml ([258a828](258a828))
* rename triage.yml ([3b9a14b](3b9a14b))
* undefined config during build ([#158](#158)) ([c986086](c986086))

### πŸ• Features

* latest highlights ([#154](#154)) ([0ccd0e7](0ccd0e7))
* uses username on highlight instead of full name ([#162](#162)) ([801fe5a](801fe5a))
@github-actions
Copy link

github-actions bot commented Jun 3, 2023

πŸŽ‰ This PR is included in version 1.4.0 πŸŽ‰

The release is available on GitHub release

Your semantic-release bot πŸ“¦πŸš€

zer0and1 pushed a commit to zer0and1/open-sauced.ai that referenced this pull request Jul 26, 2023
## [1.3.1-beta.4](open-sauced/ai@v1.3.1-beta.3...v1.3.1-beta.4) (2023-05-29)

### πŸ› Bug Fixes

* Post a highlight ([#101](open-sauced/ai#101)) ([e3b7051](open-sauced/ai@e3b7051))
zer0and1 pushed a commit to zer0and1/open-sauced.ai that referenced this pull request Jul 26, 2023
## [1.4.0](open-sauced/ai@v1.3.0...v1.4.0) (2023-06-03)

### βœ… Tests

* add tests for checkAuth.ts ([#149](open-sauced/ai#149)) ([3a79fac](open-sauced/ai@3a79fac))
* add tests for utils/urlMatchers.ts ([#143](open-sauced/ai#143)) ([26702e6](open-sauced/ai@26702e6))

### πŸ› Bug Fixes

* Post a highlight ([#101](open-sauced/ai#101)) ([e3b7051](open-sauced/ai@e3b7051))
* remove auto-take message from triage.yml ([258a828](open-sauced/ai@258a828))
* rename triage.yml ([3b9a14b](open-sauced/ai@3b9a14b))
* undefined config during build ([#158](open-sauced/ai#158)) ([c986086](open-sauced/ai@c986086))

### πŸ• Features

* latest highlights ([#154](open-sauced/ai#154)) ([0ccd0e7](open-sauced/ai@0ccd0e7))
* uses username on highlight instead of full name ([#162](open-sauced/ai#162)) ([801fe5a](open-sauced/ai@801fe5a))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: add PR to highlights from the extension
4 participants