New diff notes
Fixes #12732 (closed), #14731 (closed), #19375 (closed), #14783 (closed)
Builds on https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4110
To do:
-
Get it mostly working -
Validate position validity -
Fix: Don’t link to #
-
Fix: Base ref can be nil
, potentially, when the MR has an oprhan source branch => Yep, doesn’t work. We need to store astart_id
-
Optimize: Fewer duplicate git diff
compares -
Optimize: Pass paths to PositionTracer#diff
for faster diffs -
Refactor: Use head_id
inMergeRequest
/MergeRequestDiff
instead ofsource_sha
-
Refactor: Convert existing array-based diff refs to the DiffRefs model -
Tweak: Use note_type
inAutosave
key -
Tweak: Remove line_code: note.line_code
fromlink_to_reply_discussion
-
Update: SentNotifications
and reply-by-email receiver -
Update: MR diff notification email -
Update: API (MR, Commit note creation and entity) -
Update: GitHub importer -
Address any other TODO comments -
Fix: Suppress "edited 4 minutes ago" -
Write tests -
LineMapper
-
PositionTracer
-
Position
-
DiffPositionUpdateService
-
DiffNote
-
MergeRequests::RefreshService
/MergeRequest#update_diff_notes_positions
-
-
Make sure commits with diff notes don't get cleaned up, since this would prevent the diff notes from being rendered (https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5062)
Future improvements:
- Display unresolved comments on files outside the diff, if the comment was added when that file was part of the diff
- Allow commenting on sections between hunks, when expanding the diff using
...
- (We'd need to generate line code based on Position if we have it, even if it falls outside bounds of diff)
-
diff_hunk
on diff note API entity - Show diff hunk in notification email
- Resolved line notes would have a boolean, and be inactive through
notes.any? { !active? || resolved? }
- Multi line notes would store a number of positions, and do the right thing (™) in grouping and then rendering if the first item is multiline? => true
- Image diff notes could store x,y,width,height instead of old_line,new_line for similar grouping. Does it need a reference to say if it's on old or new? These can't have line_codes, clearly. Rendering would be interesting.
- Show commit line comments in the MR diff
- Comment on specific selected words
- Comment on file header
- Unfold top of discussion diff note
- New diff notes API for commits and MRs
/cc @rspeicher