[BUG] Danger doesn't add a comment if the commented line is not part of the diff

[f-meloni]

Describe the bug If your Dangerfile generates an inline comment, which points to a line which not part of your current PR's diff, Danger doesn't post anything on the PR. The inline comment API doesn't work if the item is not part of the diff, however Danger used to gather all the failed inline comments, and post them as part of the main Danger comment.

To Reproduce Steps to reproduce the behavior:

1. Create a Dangerfile which comments at line 2 of a random file
2. Change a different file
3. Open a PR

Expected behavior Danger posts a comment with the failure

Your Environment

software	version
danger.js	11.0.2
danger-swift	3.12.1
Operating System	MacOS

Additional context I've investigated it a little bit and saw that a new inline logic is been introduced https://github.com/danger/danger-js/pull/1176 and that might be related

[fbartho]

What CI are you testing against?

This definitely used to work on Github, but my team hasn't generated this kind of issue in a while.

[carocad]

This is also happening to us; We are using

• danger.js ^11.0.2
• OS: Linux; debian based
• Jenkins CI

In my case we just get

16:34:58  + npx danger ci
16:35:16  OK 
16:35:16  
16:35:16  
16:35:16  Failing the build, there are 3 fails.

without any comments on the PR. Running npx danger local however shows

npx danger local
OK 


## Failures
Task/Handler: unregister
                    
Tasks should tell Ansible when to return ``changed``, unless the task only reads
information. To do this, set ``changed_when``, use the ``creates`` or
``removes`` argument, or use ``when`` to run the task only if another check has
a particular result.

For example, this task registers the ``shell`` output and uses the return code
to define when the task has changed.

.. code:: yaml

    - name: handle shell output with return code
      ansible.builtin.shell: cat {{ myfile|quote }}
      register: myoutput
      changed_when: myoutput.rc != 0

The following example will trigger the rule since the task does not
handle the output of the ``command``.

.. code:: yaml

    - name: does not handle any output or return codes
      ansible.builtin.command: cat {{ myfile|quote }}
    
-
Task/Handler: stop
                    
Tasks should tell Ansible when to return ``changed``, unless the task only reads
information. To do this, set ``changed_when``, use the ``creates`` or
``removes`` argument, or use ``when`` to run the task only if another check has
a particular result.

For example, this task registers the ``shell`` output and uses the return code
to define when the task has changed.

.. code:: yaml

    - name: handle shell output with return code
      ansible.builtin.shell: cat {{ myfile|quote }}
      register: myoutput
      changed_when: myoutput.rc != 0

The following example will trigger the rule since the task does not
handle the output of the ``command``.

.. code:: yaml

    - name: does not handle any output or return codes
      ansible.builtin.command: cat {{ myfile|quote }}
    
-
Task/Handler: uninstall
                    
Tasks should tell Ansible when to return ``changed``, unless the task only reads
information. To do this, set ``changed_when``, use the ``creates`` or
``removes`` argument, or use ``when`` to run the task only if another check has
a particular result.

For example, this task registers the ``shell`` output and uses the return code
to define when the task has changed.

.. code:: yaml

    - name: handle shell output with return code
      ansible.builtin.shell: cat {{ myfile|quote }}
      register: myoutput
      changed_when: myoutput.rc != 0

The following example will trigger the rule since the task does not
handle the output of the ``command``.

.. code:: yaml

    - name: does not handle any output or return codes
      ansible.builtin.command: cat {{ myfile|quote }}
    

Danger: ⅹ Failing the build, there are 3 fails.

[orta]

danger local posts to the CLI by default, danger ci would post to your pull request and not the CLI

[carocad]

@orta yes, I know. I was providing the output to show that the output is there but it is not been shown in the PR. Hope it helps.

[fbartho]

I'm seeing this issue in our repo as well. Poking around to see if I can figure out why it's not working.

[ThomasGHenry]

Similar thing for us. fail() prints to the github action output, but the status remains successful and there's no comment on the PR. This used to work. I'm not sure when it stopped.

[f-meloni]

It might be (just a guess) that before we were trying to post the comments one by one, so if one was failing, that was added to the Danger "global" comment, while now it tries to make only one single review with all the comments, so if it fails to write some of the comments, the API request doesn't fail so the failing comments are not added to the global comment anymore.

[fbartho]

@f-meloni I think your guess feels really probable.

Is there any way to resolve this (via changes to our API calls, or by changing the timing of our API calls so that comments get "added to the review" first, and then the final review is constructed after those succeed)? or is this a gap in the Github API?

In our repo we had some tools that were regularly adding comments to non-edited files (when a configuration change was needed due to changes elsewhere), so I was able to add a quick user-side workaround for that case, but that still leaves a gap for cases like when you add a new eslint warning that triggers elsewhere in a file that you did actually edit (those comments would still be missing). There's a way for us to fix this by checking if every line you try to comment on is present in one of the git-dsl's diff chunks, but I worry that that would materially slow down the community. -- I'm not sure we're prefetching all the diff chunks for every source file. -- If the git-diff-chunk fetching is aggressively cached, maybe this won't be a problem, as you'd only pay the price once per file you need to comment on, which I think is fair.

What do you think @orta? Would aggressively caching git-diff-chunk fetching + pre-filtering all "inline" comments to see if they're a valid comment target be acceptable?

[orta]

What do you think @orta? Would aggressively caching git-diff-chunk fetching + pre-filtering all "inline" comments to see if they're a valid comment target be acceptable?

I think we're already doing this - https://github.com/danger/danger-js/blob/main/source/platforms/github/comms/issueCommenter.ts#L8-L43

[fbartho]

If fetching all diff chunks is aggressively cached, then the code that decides to post a review could test every file/line combo for candidate inline comments & move them into the main report if the comment is likely to fail.

I think that’s reasonable for the DangerJS GitHub provider to implement. Any concerns? (Performance?)

[orta]

An alternative method is to look for failing requests for each inline comment and then paste those into the main thread, which I think is how it is implemented in danger ruby

[Rouby]

I just took a look at the code I wrote and I think the bug is that failing inline reviews don't get added to the "leftover" inline result array. I have created a PR that should address this.

[carocad]

I can confirm that version 11.0.4 fixes the issue on our end.

[airtonix]

It's super unfortunate that a patch release changed the version of nodejs it means we're excluded from this fix. Looking through the PR i can't see anything that would require a nodejs version change.

[fbartho]

Closing this ticket since it appears resolved as of April 28, 2022 https://github.com/danger/danger-js/issues/1207#issuecomment-1111897695