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: Handle first pull request experience #2288

Merged
merged 14 commits into from
Sep 27, 2023
Merged

feat: Handle first pull request experience #2288

merged 14 commits into from
Sep 27, 2023

Conversation

RulaKhaled
Copy link
Contributor

@RulaKhaled RulaKhaled commented Sep 25, 2023

Description

Mostly meant to identify the first PR in the pull request page, display the welcome banner, handle the new type in files changed and indirect changed tab.

Notable Changes

  • New welcome banner (TS!!)
  • Handle different types in pull request content
  • Refactor of tests + new tests

Note

Waiting on Kyle's review for some copy things

Screenshots

Screenshot 2023-09-27 at 7 10 29 PM Screenshot 2023-09-27 at 7 10 25 PM

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

RulaKhaled and others added 8 commits September 24, 2023 16:03
…2283)

Update queries and zod schemas to handle FirstPullRequest on comparison.
Update the useCommit and useCompareTotals hook to typescript, use the repository gql field, and accept a filters 
argument to pass along with the query. Create a new typesafe mapEdges function.

GH codecov/engineering-team#343
@codecov-staging
Copy link

codecov-staging bot commented Sep 25, 2023

Codecov Report

Merging #2288 (674d85e) into main (adb9452) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2288   +/-   ##
=======================================
  Coverage   98.01%   98.02%           
=======================================
  Files         703      704    +1     
  Lines        8178     8195   +17     
  Branches     1924     1977   +53     
=======================================
+ Hits         8016     8033   +17     
  Misses        160      160           
  Partials        2        2           
Files Coverage Δ
...ullRequestPage/FirstPullBanner/FirstPullBanner.tsx 100.00% <100.00%> (ø)
src/pages/PullRequestPage/PullRequestPage.jsx 100.00% <100.00%> (ø)
.../PullRequestPageContent/PullRequestPageContent.jsx 100.00% <100.00%> (ø)
...rc/pages/PullRequestPage/hooks/usePullPageData.tsx 100.00% <ø> (ø)
...gedTab/FilesChanged/hooks/useImpactedFilesTable.js 100.00% <ø> (ø)
...tPage/subroute/FilesChangedTab/FilesChangedTab.jsx 100.00% <100.00%> (ø)
...ChangedFiles/hooks/useIndirectChangedFilesTable.js 100.00% <100.00%> (ø)
...subroute/IndirectChangesTab/IndirectChangesTab.jsx 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adb9452...674d85e. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Merging #2288 (674d85e) into main (adb9452) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2288   +/-   ##
=====================================
  Coverage   98.02   98.02           
=====================================
  Files        703     704    +1     
  Lines       8178    8195   +17     
  Branches    1965    1977   +12     
=====================================
+ Hits        8016    8033   +17     
  Misses       160     160           
  Partials       2       2           
Files Coverage Δ
...ullRequestPage/FirstPullBanner/FirstPullBanner.tsx 100.00% <100.00%> (ø)
src/pages/PullRequestPage/PullRequestPage.jsx 100.00% <100.00%> (ø)
.../PullRequestPageContent/PullRequestPageContent.jsx 100.00% <100.00%> (ø)
...rc/pages/PullRequestPage/hooks/usePullPageData.tsx 100.00% <ø> (ø)
...gedTab/FilesChanged/hooks/useImpactedFilesTable.js 100.00% <ø> (ø)
...tPage/subroute/FilesChangedTab/FilesChangedTab.jsx 100.00% <100.00%> (ø)
...ChangedFiles/hooks/useIndirectChangedFilesTable.js 100.00% <100.00%> (ø)
...subroute/IndirectChangesTab/IndirectChangesTab.jsx 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adb9452...674d85e. Read the comment docs.

@netlify
Copy link

netlify bot commented Sep 25, 2023

Deploy Preview for gazebo-staging ready!

Name Link
🔨 Latest commit fa9b1a4
🔍 Latest deploy log https://app.netlify.com/sites/gazebo-staging/deploys/6511c8d11d3f180008233ea6
😎 Deploy Preview https://deploy-preview-2288--gazebo-staging.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@codecov-public-qa
Copy link

codecov-public-qa bot commented Sep 25, 2023

Codecov Report

Merging #2288 (674d85e) into main (adb9452) will decrease coverage by 0.07%.
The diff coverage is 5.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2288      +/-   ##
==========================================
- Coverage   34.32%   34.26%   -0.07%     
==========================================
  Files         654      655       +1     
  Lines        8030     8047      +17     
  Branches     1922     1970      +48     
==========================================
+ Hits         2756     2757       +1     
- Misses       5251     5261      +10     
- Partials       23       29       +6     
Files Changed Coverage Δ
...ullRequestPage/FirstPullBanner/FirstPullBanner.tsx 0.00% <0.00%> (ø)
src/pages/PullRequestPage/PullRequestPage.jsx 0.00% <0.00%> (ø)
.../PullRequestPageContent/PullRequestPageContent.jsx 0.00% <0.00%> (ø)
...rc/pages/PullRequestPage/hooks/usePullPageData.tsx 82.35% <ø> (ø)
...gedTab/FilesChanged/hooks/useImpactedFilesTable.js 100.00% <ø> (ø)
...tPage/subroute/FilesChangedTab/FilesChangedTab.jsx 0.00% <0.00%> (ø)
...subroute/IndirectChangesTab/IndirectChangesTab.jsx 0.00% <0.00%> (ø)
...ChangedFiles/hooks/useIndirectChangedFilesTable.js 100.00% <100.00%> (ø)

... and 6 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adb9452...674d85e. Read the comment docs.

@netlify
Copy link

netlify bot commented Sep 25, 2023

Deploy Preview for gazebo-staging ready!

Name Link
🔨 Latest commit 674d85e
🔍 Latest deploy log https://app.netlify.com/sites/gazebo-staging/deploys/651474396f520d0008928b6c
😎 Deploy Preview https://deploy-preview-2288--gazebo-staging.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@RulaKhaled RulaKhaled marked this pull request as ready for review September 27, 2023 14:25
Copy link
Contributor

@adrian-codecov adrian-codecov left a comment

Choose a reason for hiding this comment

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

Left some smol qs

@@ -68,7 +68,6 @@ query PullPageData($owner: String!, $repo: String!, $pullId: Int!) {
repository(name: $repo) {
__typename
... on Repository {
Copy link
Contributor

Choose a reason for hiding this comment

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

Confirming we no longer use private

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, i just missed it when i cleaned up the use of current user part of org


if (isLoading) {
return <Loader />
}

if (isFirstPull) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I've seen Nick start to do some of the error handling at the hook level, for example here https://github.com/codecov/gazebo/blob/main/src/services/pulls/usePulls.tsx#L235, that I think it's a nice way to clean up the react code we see. I don't think this is necessarily the time to make this change here since you'd have some error handling at the hook level and the rest here, but more of a food for thought to keep this in mind for future work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm nice, could be good to clean up later

provider: string
owner: string
repo: string
pullId: string
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't pullId an int?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not in the query params

@RulaKhaled RulaKhaled merged commit 890aff6 into main Sep 27, 2023
29 of 31 checks passed
@RulaKhaled RulaKhaled deleted the new-first-pr-type branch September 27, 2023 18:42
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.

None yet

3 participants