[BUG] --removePreviousComments overrides resolved threads on Gitlab

[calto001]

Describe the bug When running danger ci with the flag --removePreviousComments it will re-add inline comments that have been resolved. For instance if we create a inline message on the same line as a changed file then danger will create a thread with our message/suggestion. If a developer resolves the thread by pressing the button and not fixing the code then on next run danger will re add the thread.

To Reproduce Steps to reproduce the behavior:

1. Create an inline message on the same line as a modified file.
2. Run danger ci --removePreviousComments
3. Go to the PR and see the message has created a thread on the line change
4. Manually press resolve thread
5. Re run danger
6. See that the resolved thread has re appeared

Expected behavior When using the --removePreviousComments flag danger should not re add resolved threads that have been created via an inline message.

Screenshots

Your Environment

software	version
danger.js	11.1.4
node	18
Gitlab Self Hosted	15.11
CI	Gitlab

Additional context Not sure if this a bug or more of a feature request in which manually resolved threads are not re added.

[orta]

Yeah, this sounds like a feature request - I've never heard of this feature on gitlab. Assuming a well tested PR, I'd be open to it in Danger

[fbartho]

I actually disagree with this feature request!

If a DangerJS comment was resolved without changing the code, of course the inline-comment is supposed to be re-added. I think your described behavior is working as expected.

The resolution status of a comment thread isn't something we can take into account to decide if a DangerJS warn(…) should be suppressed.

[calto001]

@fbartho I 100% understand your point and I wasn't sure if that was something that should or shouldn't be taken into account. I know that ultimately I am the one who writes what code that should or shouldn't trigger a warning. However, there can be a lot of use cases where a reviewer/developer has opted to resolve the thread without changing the code.

Yes we could modify our own dangerfile to limit the amount of "false-postives", but there will always be outliers. And I think it would be nice to have a way in which we can still consistently keep messages at the top without overriding manually resolved threads. Whether this is a new flag or something else.

[fbartho]

@calto001 Do you have a specific example of the type of warning/inline message you're posting? Maybe that would help me better understand your use-case and better suggest a way to resolve it?

We have some cases like that, where it's an eslint-error that we want to disregard (rarely, but sometimes). In that case we add an eslint-ignore comment targeting just that rule and just the next line.

Anything that triggers a DangerJS merge-blocking-comment for us would also trigger it on some later PR that happens to edit the same line, so a code-change has to happen or other devs would be affected when it's not their fault.

[calto001]

@fbartho In my case I want to ensure that console.log()messages are not being accidentally pushed. So when when danger runs it will leave a in-line suggestion to remove that code. However, there can be times when a developer needs to have that in there because things need to be tested in an integration or staging environment.

If we don't use the flag --removePreviousComments then the manually resolved suggestion will stay resolved but the general messages will continue to be pushed further down at the PR (lessening the chances they are read). But if we use --removePreviousComments then general messages will be at the top but manually resolved threads will be re opened.

[fbartho]

@calto001 in our repo, we use https://eslint.org/docs/latest/rules/no-console, and then if we specifically want to keep the console logs for production we allow the dev to disable that rule on a case-by-case basis.

[orta]

Yeah, I agree, with @fbartho - Danger probably shouldn't be getting influenced by external features like that