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(suspensive.org): add defense code for github contributors fetching #1054

Merged
merged 3 commits into from
Jul 8, 2024
Merged

fix(suspensive.org): add defense code for github contributors fetching #1054

merged 3 commits into from
Jul 8, 2024

Conversation

saul-atomrigs
Copy link
Contributor

Overview

Issue: #1051

I was able to reproduce the bug only by the time this issue was posted. Afterwards, I tried clearing the cache, browsing in secret mode, and even in another macbook (which never visited the page before) and in several browsers (chrome, safari, opera, edge), I couldn't reproduce the bug again... for now I added some defense code to where the error log pointed:

Captura de pantalla 2024-07-08 a las 8 02 06 p  m

I'll keep an eye on this issue.

PR Checklist

  • [✅] I did below actions if need
  1. I read the Contributing Guide
  2. I added documents and tests.

@saul-atomrigs saul-atomrigs requested a review from manudeli as a code owner July 8, 2024 11:11
Copy link

changeset-bot bot commented Jul 8, 2024

⚠️ No Changeset found

Latest commit: aa604de

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Jul 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
suspensive.org ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 8, 2024 1:07pm
v1.suspensive.org ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 8, 2024 1:07pm
visualization.suspensive.org ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 8, 2024 1:07pm

Copy link

vercel bot commented Jul 8, 2024

@saul-atomrigs is attempting to deploy a commit to the Toss Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

codspeed-hq bot commented Jul 8, 2024

CodSpeed Performance Report

Merging #1054 will create unknown performance changes

Comparing saul-atomrigs:fix/fetch-github-contributors (aa604de) with main (a84bfb2)

Summary

⚠️ No benchmarks were detected in both the base of the PR and the PR.

@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.09%. Comparing base (a84bfb2) to head (aa604de).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1054   +/-   ##
=======================================
  Coverage   84.09%   84.09%           
=======================================
  Files          49       49           
  Lines         547      547           
  Branches      118      118           
=======================================
  Hits          460      460           
  Misses         82       82           
  Partials        5        5           
Components Coverage Δ
@suspensive/react 97.03% <ø> (ø)
@suspensive/react-query 75.28% <ø> (ø)
@suspensive/react-query-4 0.00% <ø> (ø)
@suspensive/react-query-5 0.00% <ø> (ø)
@suspensive/jotai 0.00% <ø> (ø)
@suspensive/react-await 100.00% <ø> (ø)
@suspensive/react-image 80.39% <ø> (ø)

Comment on lines 44 to 56
if (Array.isArray(data) && data.length) {
const filteredContributors = data.filter((contributor) => {
const login = contributor.author.login
return !['github-actions[bot]', 'dependabot[bot]', 'renovate[bot]'].includes(login)
})
setResult({
data: filteredContributors,
error: undefined,
isError: false,
isLoading: false,
isSuccess: true,
})
}
Copy link
Member

Choose a reason for hiding this comment

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

Even if the data is not an array or the length is 0, the fetch success status must be expressed as state. Please modify isSuccess to be true so that the correct server status state can be displayed even if the length of the data is 0 or there is no data when the fetch is successful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you, I've just pushed the change
It will keep isSuccess true for all 200 OK, even though the response data is empty or non-array
plus, I refactored the code for better exception messages

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

@manudeli manudeli merged commit b4ee908 into toss:main Jul 8, 2024
14 checks passed
manudeli pushed a commit that referenced this pull request Aug 3, 2024
#1054)

# Overview

<!--
    A clear and concise description of what this pr is about.
 -->

Issue: #1051

I was able to reproduce the bug only by the time this issue was posted.
Afterwards, I tried clearing the cache, browsing in secret mode, and
even in another macbook (which never visited the page before) and in
several browsers (chrome, safari, opera, edge), I couldn't reproduce the
bug again... for now I added some defense code to where the error log
pointed:

![Captura de pantalla 2024-07-08 a las 8 02 06 p
 m](https://github.com/toss/suspensive/assets/82362278/5e1b6343-e6aa-4dad-a102-35b08dd85d47)



I'll keep an eye on this issue. 

## PR Checklist

- [✅] I did below actions if need

1. I read the [Contributing
Guide](https://github.com/toss/suspensive/blob/main/CONTRIBUTING.md)
2. I added documents and tests.
manudeli pushed a commit that referenced this pull request Aug 3, 2024
#1054)

# Overview

<!--
    A clear and concise description of what this pr is about.
 -->

Issue: #1051

I was able to reproduce the bug only by the time this issue was posted.
Afterwards, I tried clearing the cache, browsing in secret mode, and
even in another macbook (which never visited the page before) and in
several browsers (chrome, safari, opera, edge), I couldn't reproduce the
bug again... for now I added some defense code to where the error log
pointed:

![Captura de pantalla 2024-07-08 a las 8 02 06 p
 m](https://github.com/toss/suspensive/assets/82362278/5e1b6343-e6aa-4dad-a102-35b08dd85d47)



I'll keep an eye on this issue. 

## PR Checklist

- [✅] I did below actions if need

1. I read the [Contributing
Guide](https://github.com/toss/suspensive/blob/main/CONTRIBUTING.md)
2. I added documents and tests.
manudeli pushed a commit that referenced this pull request Aug 3, 2024
#1054)

# Overview

<!--
    A clear and concise description of what this pr is about.
 -->

Issue: #1051

I was able to reproduce the bug only by the time this issue was posted.
Afterwards, I tried clearing the cache, browsing in secret mode, and
even in another macbook (which never visited the page before) and in
several browsers (chrome, safari, opera, edge), I couldn't reproduce the
bug again... for now I added some defense code to where the error log
pointed:

![Captura de pantalla 2024-07-08 a las 8 02 06 p
 m](https://github.com/toss/suspensive/assets/82362278/5e1b6343-e6aa-4dad-a102-35b08dd85d47)



I'll keep an eye on this issue. 

## PR Checklist

- [✅] I did below actions if need

1. I read the [Contributing
Guide](https://github.com/toss/suspensive/blob/main/CONTRIBUTING.md)
2. I added documents and tests.
manudeli pushed a commit that referenced this pull request Aug 3, 2024
#1054)

# Overview

<!--
    A clear and concise description of what this pr is about.
 -->

Issue: #1051

I was able to reproduce the bug only by the time this issue was posted.
Afterwards, I tried clearing the cache, browsing in secret mode, and
even in another macbook (which never visited the page before) and in
several browsers (chrome, safari, opera, edge), I couldn't reproduce the
bug again... for now I added some defense code to where the error log
pointed:

![Captura de pantalla 2024-07-08 a las 8 02 06 p
 m](https://github.com/toss/suspensive/assets/82362278/5e1b6343-e6aa-4dad-a102-35b08dd85d47)



I'll keep an eye on this issue. 

## PR Checklist

- [✅] I did below actions if need

1. I read the [Contributing
Guide](https://github.com/toss/suspensive/blob/main/CONTRIBUTING.md)
2. I added documents and tests.
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.

3 participants