Danger: Fails to render HTML table in comment correctly

[mattrobenolt]

Report

What did you do?

Ran some Danger

What did you expect to happen?

Report should have rendered HTML

What happened instead?

It does not correctly render HTML.

My hunch is that possibly the GitHub API changed not allowing this HTML anymore? It seems odd, and from what I can tell, everything looks ok.

You can view example here: https://github.com/getsentry/sentry/pull/4579#issuecomment-262026317

And the raw HTML of that comment is:

<table>
  <thead>
    <tr>
      <th width="50"></th>
      <th width="100%" data-danger-table="true" data-kind="Warning">
          1 Warning
      </th>
     </tr>
  </thead>
  <tbody>

    <tr>
      <td>:warning:</td>
      <td data-sticky="true">PR is classed as Work in Progress</td>
    </tr>
  </tbody>
</table>

<p align="right" data-meta="generated_by_danger">
  Generated by :no_entry_sign: <a href="http://danger.systems/">danger</a>
</p>

[mitsuhiko]

Found the problem. It's the extra newline after <tbody> that causes github to fail to parse it.

[dzenbot]

Did Github recently change on their end? Things were silky smooth a few days ago.

[dblock]

Seeing this in https://github.com/mongoid/mongoid-grid_fs/pull/65 as well. Suggested fix?

[marcelofabri]

I've contacted GitHub support and here's what I got:

Thanks so much for doing that! We are currently updating to a new markdown renderer, which requires a new line between markdown and HTML lines. I think this may be an issue related to that.

I've asked the team to look into it. I don't have an ETA on any changes, but I'll follow up as soon as I have news.

In the meantime, we probably could remove that extra line to fix the issue.

[kivikakk]

Hey all -- we're currently moving to a CommonMark-compliant parser, which does affect how HTML is interpreted. You can see the spec (we're currently at 0.26 0.27) for more details, but here's a test case demonstrating the issue.

Per the "HTML blocks" section of the spec:

Everything until the next blank line or end of document gets included in the HTML block.

tl;dr: removing any blank lines in HTML blocks will fix the issue going forward.

We're doing our best to transition old documents in the coming days to render properly, so existing comments made by Danger will be fixed.

[dzenbot]

Thanks @kivikakk !

[marcelofabri]

@kivikakk Thanks for commenting here! Also, I've noticed that existing comments are rendered properly (they only break if they're edited)

[dblock]

I think we can fix this in Danger, won't get to it till tomorrow, hopefully someone can beat me to it ;)

[kivikakk]

@marcelofabri: ah, yes, good point! Edited and new comments are using the new engine, and old comments are using the old one. We're gradually transitioning the whole site over to the new engine, and are updating comments as we do update them to the new engine so they don't break.

[kivikakk]

I have some time, so I'll try getting a PR up!

[JuanitoFatas]

@mattrobenolt Please try v4.0.1. Thanks @kivikakk for all the hard work!

[ajfigueroa]

v4.0.1 works great. 🎉

Out of curiosity, are new lines treated differently for warn vs fail?
I'm seeing the same formatting issue only for warns messages that contain news line characters. However, my fail messages which also contain new line characters are rendering fine.

[kivikakk]

@ajfigueroa: I suspect it's a time difference thing (whether they were posted before or after the CommonMark switch was flicked) — they should all be looking okay as of a day or so from now when the transition completes. If you have any specific examples, feel free to link and I'll look into them!