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

[Bug] image in readme would crash when default branch is not master #1268

Closed
dongxuwang opened this issue Sep 1, 2023 · 12 comments · Fixed by #1305
Closed

[Bug] image in readme would crash when default branch is not master #1268

dongxuwang opened this issue Sep 1, 2023 · 12 comments · Fixed by #1305

Comments

@dongxuwang
Copy link
Contributor

dongxuwang commented Sep 1, 2023

Current behavior

image in readme would crash when default branch is not master

see project logo in here

Expected Behavior

image in readme should display

Extra comments

Should tune Client.scala rather hard code master

Search terms

master or blob in Client.scala

@adpi2
Copy link
Member

adpi2 commented Sep 20, 2023

Thanks for the bug report.

@dongxuwang
Copy link
Contributor Author

After looking through the code, looks that's not easy to figure this out.

@adpi2
Copy link
Member

adpi2 commented Sep 21, 2023

I guess we need to use the GithubClient to get the default branch and store that into GithubInfo (which is saved in a SQL table). Then use that info to get the correct URL of images.

If you have time to look into that, it would be helpful.

@mfirry
Copy link
Contributor

mfirry commented Nov 15, 2023

what if just before this (or in parallel) call to github api

    val request = new Request(
      s"https://api.github.com/repos/$organization/$repository/readme",
      new RequestInit {
        headers = headersWithCreds.toJSDictionary
      }
    )

One makes this other one https://api.github.com/repos/$organization/$repository/ with which you can get the default_branch?

@adpi2
Copy link
Member

adpi2 commented Nov 15, 2023

One makes this other one https://api.github.com/repos/$organization/$repository/ with which you can get the default_branch?

Yes that should solve the issue! Feel free to open a PR.

@mfirry
Copy link
Contributor

mfirry commented Nov 15, 2023

I'll give it a shot!

to be fair: it's not the solution... I see it as a workaround but it should a) serve the purpose b) have little impact on the structure of that part of the codebase.

@mfirry
Copy link
Contributor

mfirry commented Nov 15, 2023

I'll try and summarize a possible solution.

As of now def fetchAndReplaceReadme(element: Element, ... does few things:

  1. It retrives the readme information
    val request = new Request(
      s"https://api.github.com/repos/$organization/$repository/readme", ...)
  1. Updates the markup for the given element (element.innerHTML = res)
  2. Checks all img and a elements and for them checks whether they have href or src attributes and then sets them to some specific values that can be github.com/$organization/$repository/raw/$branch or github.com/$organization/$repository/blob/$branch (and branch is master)

Now what I had in mind is to keep point 1 and 2 as they and separate the rest.

So the first part would look like:

    // Simplified version
    fetch(readmeRequest).toFuture
      .flatMap { res =>
        res.text().toFuture
      }
      .foreach(res => element.innerHTML = res)

The second one would make the "new" GitHub API request (repoRequest), parse the JSON and retrieve the default_branch. Only then It can go on with updating href attributes for the anchors and the images.

So something along these lines:

      // Simplified version
      @js.native
        trait Repo extends js.Object {
        val default_branch: String = js.native
      }

(...)

      val repoRequest = new Request(
        s"https://api.github.com/repos/$organization/$repository/", ...)

      fetch(repoRequest).toFuture
        .flatMap(res => res.text().toFuture)
          .foreach { res =>
            val resJson = js.JSON.parse(res)
            val branch = resJson.asInstanceOf[Repo].default_branch
           setImagesAndAnchors(branch, organization, repository)
        }

(And obviously setImagesAndAnchors does what it says it does)

How does this look like?

@adpi2
Copy link
Member

adpi2 commented Nov 15, 2023

How does this look like?

Looks good but the second part (or more precisely the setImagesAndAnchors) should not happen before the first part finishes. So there should be some map or flatMap to synchronize them.

@mfirry
Copy link
Contributor

mfirry commented Nov 16, 2023

Cool. I'm just wondering if I've got an easy way to test that this works. Any hint?

@adpi2
Copy link
Member

adpi2 commented Nov 16, 2023

Cool. I'm just wondering if I've got an easy way to test that this works. Any hint?

This is how you can run Scaladex locally: https://github.com/scalacenter/scaladex/blob/main/CONTRIBUTING.md#run-scaladex-locally. Then you should go to the scalacenter/bloop project and see if the image appears.

@mfirry
Copy link
Contributor

mfirry commented Nov 21, 2023

Working on it have a look mfirry@d960fa3

still trying to understand if can i somehow handle unsuccessful API calls

@adpi2
Copy link
Member

adpi2 commented Nov 21, 2023

@mfirry thanks for the update. It is looking good.

still trying to understand if can i somehow handle unsuccessful API calls

The API call should not fail and if it fails I would use master as the default branch.

@mfirry mfirry mentioned this issue Nov 21, 2023
mfirry added a commit to mfirry/scaladex that referenced this issue Nov 22, 2023
@adpi2 adpi2 linked a pull request Nov 22, 2023 that will close this issue
mfirry added a commit to mfirry/scaladex that referenced this issue Nov 22, 2023
mfirry added a commit to mfirry/scaladex that referenced this issue Nov 22, 2023
adpi2 added a commit that referenced this issue Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants