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: clean and release PR Lookup #26

Merged
merged 6 commits into from
Dec 14, 2023
Merged

feat: clean and release PR Lookup #26

merged 6 commits into from
Dec 14, 2023

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Dec 13, 2023

Closes #25

This PR finishes an in-progress effort to display PR information to Electron developers. Being able to see PR information allows them to more easily determine if a given fix is in a version they've consumed without needing to peruse release notes, among other things.

There's definitely better error validation and other things to add to this but given folks are about to head to holiday i'd like to get this out the door before we all do! Some of the data parsing is also a bit off but this feature is intended to be a UI change.

Designed to take after https://chromiumdash.appspot.com/

PR Lookup View:

Screenshot 2023-12-13 at 9 30 27 PM

New PR View:

Screenshot 2023-12-14 at 12 19 08 PM

@codebytere codebytere marked this pull request as ready for review December 13, 2023 20:30
@codebytere codebytere requested review from a team as code owners December 13, 2023 20:30
@codebytere codebytere force-pushed the spruce-pr-lookup branch 3 times, most recently from 106a8dc to 9a5b085 Compare December 13, 2023 20:36
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

JS LGTM.

I'm fine with the UI too, but usually stay away from UI / CSS reviews. If in doubt, maybe get a 2nd opinion on that part 😸

Either way, I like that the new CSS is only about half the size of the old 🔪

Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

Overall excited for this, but a few fixes, and I'm worried about some security implications (see inline comment).

I think we should also bump the cache time on getPR and getPRComments which are both less than a minute currently - cold load time for a PR is fairly slow. Seems reasonable to cache these for longer - I'd be fine with an hour?

src/static/css/pr.css Show resolved Hide resolved
src/routes/pr.js Outdated Show resolved Hide resolved
src/routes/pr.js Outdated Show resolved Hide resolved
src/views/pr.handlebars Outdated Show resolved Hide resolved
Co-authored-by: David Sanders <[email protected]>
@codebytere codebytere merged commit e837e20 into main Dec 14, 2023
2 checks passed
@codebytere codebytere deleted the spruce-pr-lookup branch December 14, 2023 16:47
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.

Clean up and release PR tracking view
3 participants