Create a components selector that fetches components by head commit of a branch #2452

rohitvinnakota-codecov · 2023-12-12T16:31:54Z

Description

Part 1 of a multi-phase change to add the components selector to the file viewer on the coverage tab. In this PR, the selector component is created. It fetches components from the head commit of a given branch and supports filtering. Note that the selector itself is not being used as of yet. This will be addressed in a follow up PR with changes to Fileviewer and related files. This PR addresses codecov/engineering-team#832.

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.

codecov

In this review, I've identified significant concerns in both implementation and design. Many points are related to maintaining statically-typed nature of your codebase. There are a few places where any type was used, which should be avoided when TypeScript is being used. In addition, several ts-expect-error directives were used either due to lacking type definitions or to suppress type warnings, and a use of undefined under UseBranchComponentsArgs. While it might seem convenient to suppress TypeScript's type checking or to use any or undefined, these practices may disguise bugs or other issues for the static type-checker and lead to maintenance difficulties later on.

codecov · 2023-12-12T16:32:48Z

src/pages/RepoPage/CoverageTab/subroute/Fileviewer/ComponentsSelectCommit.spec.tsx

+jest.mock('../../summaryHooks')
+
+const mockedUseFlags = useFlags as jest.Mock<{ componentsSelect: boolean }>
+const mockedUseSummary = useSummary as jest.Mock<any>


The 'useSummary' hook is mocked globally for all tests with the option to modify it per test case. It's possible for the return values of these mock functions to be inadvertently carried into other tests, causing them to rely on each other’s state and affect their execution. Consider avoiding the global mocking or investigate strategies to isolate the test cases.

I did this to avoid defining and writing out all attributes with their types and subtypes since we only care about one attribute in this case.

src/pages/RepoPage/CoverageTab/subroute/Fileviewer/ComponentsSelectCommit.spec.tsx

src/pages/RepoPage/CoverageTab/subroute/Fileviewer/ComponentsSelectCommit.tsx

codecov · 2023-12-12T16:32:48Z

src/services/branches/useBranchComponents.tsx

@@ -46,7 +46,7 @@ interface UseBranchComponentsArgs {
  provider: string
  owner: string
  repo: string
-  branch: string
+  branch: string | undefined


It is always recommended to avoid types such as undefined in TypeScript. It tends to invite a number of runtime type errors.

Can you swap this to branch?: string to make it optional over using undefined directly

src/pages/RepoPage/CoverageTab/subroute/Fileviewer/ComponentsSelectCommit.tsx

codecov-qa · 2023-12-12T16:35:13Z

Codecov Report

Merging #2452 (6623477) into main (aa54260) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2452   +/-   ##
=======================================
  Coverage   98.09%   98.10%           
=======================================
  Files         767      768    +1     
  Lines        9786     9819   +33     
  Branches     2479     2486    +7     
=======================================
+ Hits         9600     9633   +33     
  Misses        184      184           
  Partials        2        2

Files	Coverage Δ
...Tab/subroute/Fileviewer/ComponentsSelectCommit.tsx	`100.00% <100.00%> (ø)`

Components	Coverage Δ
Assets	`59.37% <ø> (ø)`
Layouts	`97.30% <ø> (ø)`
Pages	`98.96% <100.00%> (+<0.01%)`	⬆️
Services	`99.44% <ø> (ø)`
Shared	`99.81% <ø> (ø)`
UI	`94.32% <ø> (ø)`

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 aa54260...6623477. Read the comment docs.

codecov · 2023-12-12T16:35:23Z

Codecov Report

Merging #2452 (6623477) into main (aa54260) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #2452     +/-   ##
=======================================
+ Coverage   98.10   98.11   +0.01     
=======================================
  Files        767     768      +1     
  Lines       9786    9819     +33     
  Branches    2460    2491     +31     
=======================================
+ Hits        9600    9633     +33     
  Misses       184     184             
  Partials       2       2

Files	Coverage Δ
...Tab/subroute/Fileviewer/ComponentsSelectCommit.tsx	`100.00% <100.00%> (ø)`

Components	Coverage Δ
Assets	`59.37% <ø> (ø)`
Layouts	`97.30% <ø> (ø)`
Pages	`98.96% <100.00%> (+<0.01%)`	⬆️
Services	`99.44% <ø> (ø)`
Shared	`99.81% <ø> (ø)`
UI	`94.32% <ø> (ø)`

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 aa54260...6623477. Read the comment docs.

codecov-public-qa · 2023-12-12T16:35:29Z

Codecov Report

Merging #2452 (6623477) into main (aa54260) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2452   +/-   ##
=======================================
  Coverage   98.09%   98.10%           
=======================================
  Files         767      768    +1     
  Lines        9786     9819   +33     
  Branches     2474     2440   -34     
=======================================
+ Hits         9600     9633   +33     
  Misses        184      184           
  Partials        2        2

Files	Coverage Δ
...Tab/subroute/Fileviewer/ComponentsSelectCommit.tsx	`100.00% <100.00%> (ø)`

Components	Coverage Δ
Assets	`59.37% <ø> (ø)`
Layouts	`97.30% <ø> (ø)`
Pages	`98.96% <100.00%> (+<0.01%)`	⬆️
Services	`99.44% <ø> (ø)`
Shared	`99.81% <ø> (ø)`
UI	`94.32% <ø> (ø)`

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 aa54260...6623477. Read the comment docs.

netlify · 2023-12-12T16:36:28Z

✅ Deploy Preview for gazebo-staging ready!

Name	Link
🔨 Latest commit	`6623477`
🔍 Latest deploy log	https://app.netlify.com/sites/gazebo-staging/deploys/6579c9b5cbf87a0008dc507b
😎 Deploy Preview	https://deploy-preview-2452--gazebo-staging.netlify.app/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

nicholas-codecov

Few quick things to peak at 👀

nicholas-codecov · 2023-12-12T17:12:42Z

src/pages/RepoPage/CoverageTab/subroute/Fileviewer/ComponentsSelectCommit.spec.tsx

+jest.mock('../../summaryHooks')
+
+const mockedUseFlags = useFlags as jest.Mock<{ componentsSelect: boolean }>
+const mockedUseSummary = useSummary as jest.Mock<any>


Wondering about the justification for mocking out this hook, over using msw? We typically want to avoid this so we have a deeper integration test, as well if any changes are ever made to useSummary that break the functionality we won't pick it up.

My initial thoughts were mocking all the hooks within useSummary() would have been overkill, but I agree with your point about not picking up changes to useSummary. Updated.

nicholas-codecov · 2023-12-12T17:17:18Z

src/services/branches/useBranchComponents.tsx

@@ -46,7 +46,7 @@ interface UseBranchComponentsArgs {
  provider: string
  owner: string
  repo: string
-  branch: string
+  branch: string | undefined


Can you swap this to branch?: string to make it optional over using undefined directly

codecov

CodecovAI submitted a new review for e06c7b4

codecov · 2023-12-12T17:24:31Z

src/services/branches/useBranchComponents.tsx

@@ -46,7 +46,7 @@ interface UseBranchComponentsArgs {
  provider: string
  owner: string
  repo: string
-  branch: string
+  branch?: string
  filters?: {


If possible, you should refrain from making optional arguments that could have default values. Here 'branch' being undefined might lead to errors.

Oh yea sorry I missed this, branch is a non-nullable graphql string argument and is required to be passed at all times even if it's just an empty string.

src/pages/RepoPage/CoverageTab/subroute/Fileviewer/ComponentsSelectCommit.spec.tsx

src/pages/RepoPage/CoverageTab/subroute/Fileviewer/ComponentsSelectCommit.tsx

codecov

CodecovAI submitted a new review for e4b9ada

src/pages/RepoPage/CoverageTab/subroute/Fileviewer/ComponentsSelectCommit.spec.tsx

src/pages/RepoPage/CoverageTab/subroute/Fileviewer/ComponentsSelectCommit.tsx

codecov

CodecovAI submitted a new review for 4a24df2

src/pages/RepoPage/CoverageTab/subroute/Fileviewer/ComponentsSelectCommit.spec.tsx

nicholas-codecov

Looks all good to me 👍

sentry-io · 2023-12-17T05:26:58Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ React ErrorBoundary TypeError: Failed to fetch graphql(shared/api/api) View Issue

_{Did you find this useful? React with a 👍 or 👎}

add components selector by commit file

d77e4c0

codecov bot reviewed Dec 12, 2023

View reviewed changes

rohitvinnakota-codecov marked this pull request as ready for review December 12, 2023 17:06

rohitvinnakota-codecov changed the title ~~Create a components selector that fetches components by head commit~~ Create a components selector that fetches components by head commit of a branch Dec 12, 2023

nicholas-codecov reviewed Dec 12, 2023

View reviewed changes

use optional

e06c7b4

codecov bot reviewed Dec 12, 2023

View reviewed changes

update tests

4a24df2

rohitvinnakota-codecov force-pushed the rvinnakota-components-file-viewer branch from e4b9ada to 4a24df2 Compare December 12, 2023 17:36

codecov bot reviewed Dec 12, 2023

View reviewed changes

make branch arg required

ff764e1

nicholas-codecov approved these changes Dec 12, 2023

View reviewed changes

Merge branch 'main' into rvinnakota-components-file-viewer

6623477

rohitvinnakota-codecov merged commit 218e142 into main Dec 13, 2023
50 checks passed

rohitvinnakota-codecov deleted the rvinnakota-components-file-viewer branch December 13, 2023 15:18

rohitvinnakota-codecov mentioned this pull request Dec 13, 2023

[feat] Add components selector to file viewer #2442

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a components selector that fetches components by head commit of a branch #2452

Create a components selector that fetches components by head commit of a branch #2452

rohitvinnakota-codecov commented Dec 12, 2023 •

edited

codecov bot left a comment

codecov bot Dec 12, 2023 •

edited

rohitvinnakota-codecov Dec 12, 2023

codecov bot Dec 12, 2023

nicholas-codecov Dec 12, 2023

codecov-qa bot commented Dec 12, 2023 •

edited

codecov bot commented Dec 12, 2023 •

edited

codecov-public-qa bot commented Dec 12, 2023 •

edited

netlify bot commented Dec 12, 2023 •

edited

nicholas-codecov left a comment

nicholas-codecov Dec 12, 2023

rohitvinnakota-codecov Dec 12, 2023

nicholas-codecov Dec 12, 2023

codecov bot left a comment

codecov bot Dec 12, 2023 •

edited

nicholas-codecov Dec 12, 2023

codecov bot left a comment

codecov bot left a comment

nicholas-codecov left a comment

sentry-io bot commented Dec 17, 2023

Create a components selector that fetches components by head commit of a branch #2452

Create a components selector that fetches components by head commit of a branch #2452

Conversation

rohitvinnakota-codecov commented Dec 12, 2023 • edited

Description

Legal Boilerplate

codecov bot left a comment

Choose a reason for hiding this comment

codecov bot Dec 12, 2023 • edited

Choose a reason for hiding this comment

rohitvinnakota-codecov Dec 12, 2023

Choose a reason for hiding this comment

codecov bot Dec 12, 2023

Choose a reason for hiding this comment

nicholas-codecov Dec 12, 2023

Choose a reason for hiding this comment

codecov-qa bot commented Dec 12, 2023 • edited

Codecov Report

codecov bot commented Dec 12, 2023 • edited

Codecov Report

codecov-public-qa bot commented Dec 12, 2023 • edited

Codecov Report

netlify bot commented Dec 12, 2023 • edited

✅ Deploy Preview for gazebo-staging ready!

nicholas-codecov left a comment

Choose a reason for hiding this comment

nicholas-codecov Dec 12, 2023

Choose a reason for hiding this comment

rohitvinnakota-codecov Dec 12, 2023

Choose a reason for hiding this comment

nicholas-codecov Dec 12, 2023

Choose a reason for hiding this comment

codecov bot left a comment

Choose a reason for hiding this comment

codecov bot Dec 12, 2023 • edited

Choose a reason for hiding this comment

nicholas-codecov Dec 12, 2023

Choose a reason for hiding this comment

codecov bot left a comment

Choose a reason for hiding this comment

codecov bot left a comment

Choose a reason for hiding this comment

nicholas-codecov left a comment

Choose a reason for hiding this comment

sentry-io bot commented Dec 17, 2023

Suspect Issues

rohitvinnakota-codecov commented Dec 12, 2023 •

edited

codecov bot Dec 12, 2023 •

edited

codecov-qa bot commented Dec 12, 2023 •

edited

codecov bot commented Dec 12, 2023 •

edited

codecov-public-qa bot commented Dec 12, 2023 •

edited

netlify bot commented Dec 12, 2023 •

edited

codecov bot Dec 12, 2023 •

edited