[BUG] Inline comments often don't show up, or show incorrectly with GitLab self-hosted

[squarefrog]

Describe the bug
I'm unable to get inline comments to show up correctly, and consistently, with danger-js and GitLab. While danger correctly adds a comment at the line I'd expect, the comment text is incorrect.

To Reproduce

1. Setup a new repo
2. Add the .gitlab-ci.yml file listed below
3. Add a test file, LinterTest.swift listed below
4. Add the example dangerfile
5. Push up to main
6. Branch off main
7. Edit the LinterTest.swift, removing the comments in lines 22-26
8. Push up the change and look at the pipeline for the merge request

.gitlab-ci.yml

stages:
  - analysis

variables:
  LC_ALL: "en_US.UTF-8"
  LANG: "en_US.UTF-8"
  FF_ENABLE_JOB_CLEANUP: 'true'

before_script:
  - eval "$(/opt/homebrew/bin/brew shellenv)"
  - brew install danger/tap/danger-js

.mr:
  only:
    - merge_requests
  except:
    variables:
      - $CI_MERGE_REQUEST_TITLE=~ /^WIP:/

danger_ci:
  stage: analysis
  extends: .mr
  tags:
    - macos03 # adjust to your setup
  script:
    - echo "Running Danger"
    - DEBUG=* danger ci
  interruptible: true

LinterTest.swift

import Foundation

class LinterTest {
    var a = "ab"

    func annoyLinter() {
        // Triggers swiftformat spaceAroundOperators
        for i in 0 ... 10 {
            print(i)
        } // touch file to check linting

        // Triggers swiftlint empty_count
        _ = a.count == 0
    }

    enum State {
        case unknown
        case loading
        case finished
    }

    //enum NewState {
        //case unknown
        //case loading
        //case finished
    //}
}

dangerfile.ts

import { danger } from "danger";

warn("inline warning, line 16", "LinterTest.swift", 16);

warn("global warning")

warn("inline warning, line 22", "LinterTest.swift", 22);

warn("global warning 2, electric boogaloo")

Expected behavior
There should be one inline comment, underneath line 22 of LinterTest.swift, with the text inline warning, line 22. There should be 3 warnings on the main page of the merge request:

• inline warning, line 16 - as this line isn't part of the changed file, it may not show up at all
• global warning
• global warning 2, electric boogaloo

Screenshots
Overview

Changes tab

Your Environment

software	version
danger.js	11.2.1
node	N/A
npm	N/A
Operating System	macOS 12.6.2
GitLab self-hosted	15.3
gitlab-runner	15.3.0

[squarefrog]

Looking into this further, I think I understand what is happening. I can see the note originally getting created:

2023-01-31T11:43:21.556Z danger:GitLabAPI createMergeRequestDiscussion {
  id: 'c876fefd5f8ecb6993523da31743da1bc7efab87',
  individual_note: false,
  notes: [
    {
      id: 436868, <---------------------------- Take note of this ID
      type: 'DiffNote',
      body: '\n' +
        '<!--\n' +
        '  0 failure: \n' +
        '  1 warning:  inline warning, l...\n' +
        '  \n' +
        '  \n' +
        '  DangerID: danger-id-Danger;\n' +
        '  File: LinterTest.swift;\n' +
        '  Line: 22;\n' +
        '-->\n' +
        '\n' +
        '- :warning: inline warning, line 22',
      attachment: null,
      author: [Object],
      created_at: '2023-01-31T11:43:21.468Z',
      updated_at: '2023-01-31T11:43:21.468Z',
      system: false,
      noteable_id: 41158,
      noteable_type: 'MergeRequest',
      commit_id: null,
      position: [Object],
      resolvable: true,
      resolved: false,
      resolved_by: null,
      resolved_at: null,
      confidential: false,
      internal: false,
      noteable_iid: 3,
      commands_changes: {}
    }
  ]
}

Then later on, this same note is updated with the global warnings, which replaces the body:

2023-01-31T11:43:22.048Z danger:GitLabAPI updateMergeRequestNote {
  id: 436868, <--------------- We're updating the previously created ID, but with the global warnings
  type: 'DiffNote',
  body: '\n' +
    '<!--\n' +
    '  0 failure: \n' +
    '  1 warning:  global warning 2,...\n' +
    '  \n' +
    '  \n' +
    '  DangerID: danger-id-Danger;\n' +
    '-->\n' +
    '\n' +
    '\n' +
    '<table>\n' +
    '  <thead>\n' +
    '    <tr>\n' +
    '      <th width="50"></th>\n' +
    '      <th width="100%" data-danger-table="true">Warnings</th>\n' +
    '    </tr>\n' +
    '  </thead>\n' +
    '  <tbody><tr>\n' +
    '      <td>:warning:</td>\n' +
    '      <td>global warning 2, electic boogaloo</td>\n' +
    '    </tr>\n' +
    '  </tbody>\n' +
    '</table>\n' +
    '\n' +
    '\n' +
    '\n' +
    '<p align="right">\n' +
    '  Generated by :no_entry_sign: <a href="https://danger.systems/js">dangerJS</a> against 15ae2210acd12aed65c2d087967c4af0762e8148\n' +
    '</p>',
  attachment: null,
  author: {
    id: 10,
    username: 'bot',
    name: 'Bot',
    state: 'active',
    avatar_url: 'https://secure.gravatar.com/avatar/4da90f9ec250a529f07e53ff28b6680b?s=80&d=identicon',
    web_url: 'https://git.example.com/bot'
  },
  created_at: '2023-01-31T11:43:21.468Z',
  updated_at: '2023-01-31T11:43:21.947Z',
  system: false,
  noteable_id: 41158,
  noteable_type: 'MergeRequest',
  commit_id: null,
  position: {
    base_sha: '08b1c527dbb2c31ee48444941c59b5fb22bfcd16',
    start_sha: '08b1c527dbb2c31ee48444941c59b5fb22bfcd16',
    head_sha: '15ae2210acd12aed65c2d087967c4af0762e8148',
    old_path: 'LinterTest.swift',
    new_path: 'LinterTest.swift',
    position_type: 'text',
    old_line: null,
    new_line: 22,
    line_range: null
  },
  resolvable: true,
  resolved: false,
  resolved_by: null,
  resolved_at: null,
  confidential: false,
  internal: false,
  noteable_iid: 3,
  commands_changes: {}
}

Looking at the API code for GitLab, it seems like when deciding whether to create or update a discussion, we just pick the first discussion available:

updateOrCreateComment = async (dangerID: string, newComment: string): Promise<string> => {
    d("updateOrCreateComment", { dangerID, newComment })

    //Even when we don't set danger to create threads, we still need to delete them if someone answered to a single
    // comment created by danger, resulting in a discussion/thread. Otherwise we are left with dangling comments
    // that will most likely have no meaning out of context.
    const discussions = await this.getDangerDiscussions(dangerID)
    const firstDiscussion = discussions.shift()
    const existingNote = firstDiscussion?.notes[0]
    // Delete all notes from all other danger discussions (discussions cannot be deleted as a whole):
    await this.deleteNotes(this.reduceNotesFromDiscussions(discussions)) //delete the rest
    ...
}

Is it possible that this is a naive approach?

I'm afraid I know very little about typescript or javascript, so I'm not sure how I'd go about investigating this further to create a PR.

[squarefrog]

Interestingly, if I create a new MR, with only the inline warning, I see it briefly in GitLab before it disappears completely. Looking at the pipeline log it seems like it's created successfully, then gets deleted at the end of the pipeline.

Full Log

2023-01-31T12:09:51.033Z danger:GitLab createInlineComment {
  git: {
    base: 'bde295a159f3cacf74697cadee58992e646b0e59',
    head: 'a1335c19fcff10d63f768ab3b77668fec1c7ebf7',
    fileMatch: [Function (anonymous)],
    modified_files: [ 'LinterTest.swift' ],
    created_files: [],
    deleted_files: [],
    commits: [ [Object] ],
    diffForFile: [Function: diffForFile],
    structuredDiffForFile: [Function: structuredDiffForFile],
    JSONPatchForFile: [Function: JSONPatchForFile],
    JSONDiffForFile: [Function: JSONDiffForFile],
    linesOfCode: [Function: linesOfCode]
  },
  comment: '\n' +
    '<!--\n' +
    '  0 failure: \n' +
    '  1 warning:  inline warning, l...\n' +
    '  \n' +
    '  \n' +
    '  DangerID: danger-id-Danger;\n' +
    '  File: LinterTest.swift;\n' +
    '  Line: 22;\n' +
    '-->\n' +
    '\n' +
    '- :warning: inline warning, line 22\n' +
    '\n' +
    '\n' +
    '  ',
  path: 'LinterTest.swift',
  line: 22
}
2023-01-31T12:09:51.033Z danger:GitLabAPI getMergeRequestInfo for repo: paul.williamson/danger-test pr: 4
2023-01-31T12:09:51.180Z danger:GitLabAPI getMergeRequestInfo {
  id: 41266,
  iid: 4,
  project_id: 907,
  title: 'Test out only inline warnings',
  description: '',
  state: 'opened',
  created_at: '2023-01-31T12:09:35.935Z',
  updated_at: '2023-01-31T12:09:35.935Z',
  merged_by: null,
  merge_user: null,
  merged_at: null,
  closed_by: null,
  closed_at: null,
  target_branch: 'main',
  source_branch: 'test4',
  user_notes_count: 0,
  upvotes: 0,
  downvotes: 0,
  author: {
    id: 161,
    username: 'paul.williamson',
    name: 'Paul Williamson',
    state: 'active',
    avatar_url: 'https://git.example.com/uploads/-/system/user/avatar/161/avatar.png',
    web_url: 'https://git.example.com/paul.williamson'
  },
  assignees: [],
  assignee: null,
  reviewers: [],
  source_project_id: 907,
  target_project_id: 907,
  labels: [],
  draft: false,
  work_in_progress: false,
  milestone: null,
  merge_when_pipeline_succeeds: false,
  merge_status: 'can_be_merged',
  sha: 'a1335c19fcff10d63f768ab3b77668fec1c7ebf7',
  merge_commit_sha: null,
  squash_commit_sha: null,
  discussion_locked: null,
  should_remove_source_branch: null,
  force_remove_source_branch: true,
  reference: '!4',
  references: {
    short: '!4',
    relative: '!4',
    full: 'paul.williamson/danger-test!4'
  },
  web_url: 'https://git.example.com/paul.williamson/danger-test/-/merge_requests/4',
  time_stats: {
    time_estimate: 0,
    total_time_spent: 0,
    human_time_estimate: null,
    human_total_time_spent: null
  },
  squash: false,
  task_completion_status: { count: 0, completed_count: 0 },
  has_conflicts: false,
  blocking_discussions_resolved: true,
  approvals_before_merge: null,
  subscribed: false,
  changes_count: '1',
  latest_build_started_at: '2023-01-31T12:09:38.086Z',
  latest_build_finished_at: null,
  first_deployed_to_production_at: null,
  pipeline: {
    id: 265804,
    iid: 29,
    project_id: 907,
    sha: 'a1335c19fcff10d63f768ab3b77668fec1c7ebf7',
    ref: 'refs/merge-requests/4/head',
    status: 'running',
    source: 'merge_request_event',
    created_at: '2023-01-31T12:09:36.269Z',
    updated_at: '2023-01-31T12:09:38.089Z',
    web_url: 'https://git.example.com/paul.williamson/danger-test/-/pipelines/265804'
  },
  head_pipeline: {
    id: 265804,
    iid: 29,
    project_id: 907,
    sha: 'a1335c19fcff10d63f768ab3b77668fec1c7ebf7',
    ref: 'refs/merge-requests/4/head',
    status: 'running',
    source: 'merge_request_event',
    created_at: '2023-01-31T12:09:36.269Z',
    updated_at: '2023-01-31T12:09:38.089Z',
    web_url: 'https://git.example.com/paul.williamson/danger-test/-/pipelines/265804',
    before_sha: '0000000000000000000000000000000000000000',
    tag: false,
    yaml_errors: null,
    user: {
      id: 161,
      username: 'paul.williamson',
      name: 'Paul Williamson',
      state: 'active',
      avatar_url: 'https://git.example.com/uploads/-/system/user/avatar/161/avatar.png',
      web_url: 'https://git.example.com/paul.williamson'
    },
    started_at: '2023-01-31T12:09:38.086Z',
    finished_at: null,
    committed_at: null,
    duration: null,
    queued_duration: 1,
    coverage: null,
    detailed_status: {
      icon: 'status_running',
      text: 'running',
      label: 'running',
      group: 'running',
      tooltip: 'running',
      has_details: true,
      details_path: '/paul.williamson/danger-test/-/pipelines/265804',
      illustration: null,
      favicon: '/assets/ci_favicons/favicon_status_running-9c635b2419a8e1ec991c993061b89cc5aefc0743bb238ecd0c381e7741a70e8c.png'
    }
  },
  diff_refs: {
    base_sha: 'bde295a159f3cacf74697cadee58992e646b0e59',
    head_sha: 'a1335c19fcff10d63f768ab3b77668fec1c7ebf7',
    start_sha: 'bde295a159f3cacf74697cadee58992e646b0e59'
  },
  merge_error: null,
  first_contribution: false,
  user: { can_merge: false }
}
2023-01-31T12:09:51.181Z danger:GitLabAPI createMergeRequestDiscussion paul.williamson/danger-test 4 
<!--
  0 failure: 
  1 warning:  inline warning, l...
  
  
  DangerID: danger-id-Danger;
  File: LinterTest.swift;
  Line: 22;
-->
- :warning: inline warning, line 22
   {
  position: {
    position_type: 'text',
    base_sha: 'bde295a159f3cacf74697cadee58992e646b0e59',
    start_sha: 'bde295a159f3cacf74697cadee58992e646b0e59',
    head_sha: 'a1335c19fcff10d63f768ab3b77668fec1c7ebf7',
    old_path: 'LinterTest.swift',
    old_line: null,
    new_path: 'LinterTest.swift',
    new_line: 22
  }
}
2023-01-31T12:09:51.396Z danger:GitLabAPI createMergeRequestDiscussion {
  id: 'cfc6da95ea5561c5f1483df3e12c2e4ba100e088',
  individual_note: false,
  notes: [
    {
      id: 436909,
      type: 'DiffNote',
      body: '\n' +
        '<!--\n' +
        '  0 failure: \n' +
        '  1 warning:  inline warning, l...\n' +
        '  \n' +
        '  \n' +
        '  DangerID: danger-id-Danger;\n' +
        '  File: LinterTest.swift;\n' +
        '  Line: 22;\n' +
        '-->\n' +
        '\n' +
        '- :warning: inline warning, line 22',
      attachment: null,
      author: [Object],
      created_at: '2023-01-31T12:09:51.303Z',
      updated_at: '2023-01-31T12:09:51.303Z',
      system: false,
      noteable_id: 41266,
      noteable_type: 'MergeRequest',
      commit_id: null,
      position: [Object],
      resolvable: true,
      resolved: false,
      resolved_by: null,
      resolved_at: null,
      confidential: false,
      internal: false,
      noteable_iid: 4,
      commands_changes: {}
    }
  ]
}
2023-01-31T12:09:51.397Z danger:GitLabAPI getUser
2023-01-31T12:09:51.399Z danger:GitLab updateStatus {}
2023-01-31T12:09:51.510Z danger:GitLabAPI getUser {
  id: 10,
  username: 'bot',
  name: 'Bot',
  state: 'active',
  avatar_url: 'https://secure.gravatar.com/avatar/4da90f9ec250a529f07e53ff28b6680b?s=80&d=identicon',
  web_url: 'https://git.example.com/bot',
  created_at: '2016-09-29T15:00:36.638Z',
  bio: '',
  location: '',
  public_email: '',
  skype: '',
  linkedin: '',
  twitter: '',
  website_url: '',
  organization: '',
  job_title: '',
  pronouns: null,
  bot: false,
  work_information: null,
  followers: 0,
  following: 0,
  is_followed: false,
  local_time: '12:09 PM',
  last_sign_in_at: '2022-11-04T11:00:52.370Z',
  confirmed_at: '2016-09-29T15:00:36.638Z',
  last_activity_on: '2023-01-31',
  email: 'bot@example.com',
  theme_id: null,
  color_scheme_id: 1,
  projects_limit: 10,
  current_sign_in_at: '2022-11-14T13:24:10.395Z',
  identities: [],
  can_create_group: true,
  can_create_project: true,
  two_factor_enabled: true,
  external: false,
  private_profile: false,
  commit_email: 'bot@example.com',
  shared_runners_minutes_limit: null,
  extra_shared_runners_minutes_limit: null
}
2023-01-31T12:09:51.510Z danger:GitLabAPI getMergeRequestDiscussions paul.williamson/danger-test 4
2023-01-31T12:09:51.641Z danger:GitLabAPI getMergeRequestDiscussions [
  {
    id: 'cfc6da95ea5561c5f1483df3e12c2e4ba100e088',
    individual_note: false,
    notes: [ [Object] ]
  }
]
2023-01-31T12:09:51.641Z danger:GitLab deleteNotes { id: 436909 }
2023-01-31T12:09:51.642Z danger:GitLabAPI deleteMergeRequestNote paul.williamson/danger-test 4 436909
2023-01-31T12:09:51.800Z danger:GitLabAPI deleteMergeRequestNote true

[squarefrog]

OK after significant investigation, I've found the issue. In GitLab.ts, we have the following:

updateOrCreateComment = async (dangerID: string, newComment: string): Promise<string> => {
    d("updateOrCreateComment", { dangerID, newComment })

    //Even when we don't set danger to create threads, we still need to delete them if someone answered to a single
    // comment created by danger, resulting in a discussion/thread. Otherwise we are left with dangling comments
    // that will most likely have no meaning out of context.
    const discussions = await this.getDangerDiscussions(dangerID)
    const firstDiscussion = discussions.shift()
    const existingNote = firstDiscussion?.notes[0]
    // Delete all notes from all other danger discussions (discussions cannot be deleted as a whole):
    await this.deleteNotes(this.reduceNotesFromDiscussions(discussions)) //delete the rest

    let newOrUpdatedNote: GitLabNote

    if (existingNote) {
      // update the existing comment
      newOrUpdatedNote = await this.api.updateMergeRequestNote(existingNote.id, newComment)
    } else {
      // create a new comment
      newOrUpdatedNote = await this.createComment(newComment)
    }

    // create URL from note
    // "https://gitlab.com/group/project/merge_requests/154#note_132143425"
    return `${this.api.mergeRequestURL}#note_${newOrUpdatedNote.id}`
  }

The problem here is we update the existing inline note, with the global warnings. If I comment out the existing note check:

    // if (existingNote) {
      // update the existing comment
      // newOrUpdatedNote = await this.api.updateMergeRequestNote(existingNote.id, newComment)
    // } else {
      // create a new comment
      newOrUpdatedNote = await this.createComment(newComment)
    // }

then run from my test repo:

yarn --cwd ../danger-js build; DEBUG=* node --inspect ../danger-js/distribution/commands/danger-ci.js

I can now see both the inline and global warnings

However, I'm unsure what side effects this has, as I'm unfamiliar with the code base and flow. Does anyone have any advice?

Reverting to danger js 11.1.4 fixes this issue for us.

[DavidBrunow]

I just came across this same bug today. I'll see if I can implement the fix you mention.

[squarefrog]

Another quick fix is to downgrade to 11.1.4 which is the last release before this bug was introduced.