Skip placeholder replies in Rapid Diffs discussions

Miguel Rinconrequested to merge
628791-rapid-diffs-skip-placeholder-replies into master

Why

When a user replies to a diff discussion, the notes store (app/assets/javascripts/notes/store/legacy_notes, mutation SHOW_PLACEHOLDER_NOTE) inserts an optimistic placeholder note into the discussion: { id, isPlaceholderNote: true, placeholderType, notes: [{ body }] }. It has no author.

The Rapid Diffs discussion component, app/assets/javascripts/rapid_diffs/app/discussions/discussion_notes.vue, renders every non-system reply through noteable_note.vue, whose authorId computed reads this.author.id. For the placeholder this throws TypeError: Cannot read properties of undefined (reading 'id').

Under Vue 2 the error is caught, logged to the console, and the component renders blank. Users never saw a placeholder in Rapid Diffs. Under Vue 3 (@vue/compat) in the development build the error is rethrown and the whole discussion list breaks, so spec/features/merge_request/user_comments_on_whitespace_hidden_diff_spec.rb and user_sees_avatar_on_diff_notes_spec.rb fail. That is why the Vue 3 migration of the merge request Rapid Diffs page (!252389 (merged)) was reverted (!255026 (merged)). Issue: #628791 (closed).

The legacy discussion component (app/assets/javascripts/notes/components/discussion_notes.vue) renders placeholders through dedicated PlaceholderNote and PlaceholderSystemNote components. Rapid Diffs never had that branch.

What

  • discussion_notes.vue: the replies computed also excludes notes with isPlaceholderNote, next to the existing isDraft exclusion. One line plus a comment.
  • spec/frontend/rapid_diffs/app/discussions/discussion_notes_spec.js: two tests. Placeholder replies are not rendered through NoteableNote, and they are excluded from the ToggleRepliesWidget replies count.

How

This is the minimal fix for the crash. Rapid Diffs has never shown the optimistic placeholder, because Vue 2 rendered it blank. Skipping it keeps the visible behaviour unchanged. The saved reply appears when the store replaces the placeholder, as before. The replies toggle stops counting a phantom note.

Alternative: render the placeholder through placeholder components, as the legacy notes component does. !255075 (merged) does that with three helper methods, a dynamic <component :is> loop and two new components. That is optimistic-reply UI that Rapid Diffs never had, so it is split into a follow-up stacked on this MR: !255284 (closed). This MR unblocks the Vue 3 migration on its own.

The Vue 3 migration itself is re-applied in !255209 (closed), on top of !255205 (merged) (a non-production Vue 3 error handler). With this fix merged, the migration no longer depends on that handler.

How to verify

  • yarn jest spec/frontend/rapid_diffs/app/discussions/discussion_notes_spec.js and VUE_VERSION=3 yarn jest spec/frontend/rapid_diffs/app/discussions/discussion_notes_spec.js pass, 32 examples each.
  • Feature specs: enable the Vue 3 migration of the Rapid Diffs page. Create app/assets/javascripts/pages/projects/merge_requests/rapid_diffs/vue3_migration.yml with status: rollout and feature_flag: vue3_migrate_mr_rapid_diffs, plus the flag file config/feature_flags/beta/vue3_migrate_mr_rapid_diffs.yml, as in !255209 (closed). Run bin/rspec spec/features/merge_request/user_comments_on_whitespace_hidden_diff_spec.rb:49. It fails on master and passes with this MR.
  • Manually in GDK with the same flag: open a merge request's Changes tab, comment on a diff line, reply. Before: the reply never renders, and the console shows the TypeError followed by Cannot set properties of null (setting '__vnode'). After: the reply renders when saved, with no console error. Under Vue 2 (flag off) the behaviour is unchanged.

Screenshots or screen recordings

No visual change.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist.

References

Edited by Miguel Rincon

Merge request reports