@aturinske Could you maintainer-review this follow-up to your review notes from the linked MR?

Thomas Randolph (45c0207c) at 17 Mar 08:57

Thomas Randolph (bbfe3c3a) at 17 Mar 08:48

What does this MR do and why?

Follow-up to !226015. Addresses non-blocking review feedback from @aturinske:

• Inline the inputs variable into updateState where it is actually used
• Add data-testid attributes to the title regex toggle and fields container
• Update JS and RSpec tests to use data-testid selectors

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Thomas Randolph (75804c0d) at 17 Mar 08:24

P.S. There are merge conflicts, so I can't mark this MWPS.

Happy to return and do so when the conflicts have been resolved OR if you'd like to address the above issue here instead of a follow-up, I can re-review then, also 😄

@slashmanov Hey! Took a look through this. The overall approach looks good - the plumbing for collapse/expand and the resolve delegation to the legacy store all makes sense.

I did find one concern: updateDiscussion in discussions.js resets hidden to false on every backend update via addReactiveDiscussionProps, which is a regression from master where hidden was preserved through Object.assign. The main scenario where this surfaces is replying to a collapsed resolved thread - it'll pop back open unexpectedly. I left a detailed comment on the line in question.

I've opened #593794 (closed) to track the fix. Happy to approve this with the understanding that we'll address it in that follow-up, since it's gated behind the feature flag + querystring opt-in.

What does this MR do and why?

Adds store-level support for thread resolution in Rapid Diffs. This is a prerequisite for !226730 which adds the Vue components (resolve button UI, collapse behavior).

Changes

• discussions.js: Add collapseDiscussion / expandDiscussion actions that set both expanded (legacy diffs) and hidden (Rapid Diffs) properties
• merge_request_discussions.js: Add toggleResolveNote action that delegates to the legacy notes store, and collapseResolvedDiscussions that marks resolved discussions as hidden after initial fetch
• diff_discussions.js: Expose collapseDiscussion, expandDiscussion, and findVisibleDiscussionsForFile from the discussions store
• legacy_notes/actions.js: Remove discussionId from the internal toggleResolveNote call to avoid double-collapsing (Rapid Diffs handles collapse at the component level)
• legacy_diffs/actions.js: Guard collapseDiffDiscussion against missing diff file data — when Rapid Diffs is active, the legacy diffs store has no diff files populated

Related to !226730

This is a regression from master. On master, UPDATE_DISCUSSION does Object.assign(selectedDiscussion, { ...note }) directly - properties not in the backend payload (like hidden) are preserved. This line applies the result of addReactiveDiscussionProps, which builds { hidden: false, ...discussionData } (line 13 above). Since backend payloads never include hidden, the result always contains hidden: false, and this Object.assign overwrites the existing discussion's hidden: true with it.

On the resolve path this is masked because collapseDiscussion runs right after (related: !227220 (comment 3156830313)). But replyToDiscussion in legacy_notes/actions.js also calls UPDATE_DISCUSSION with no follow-up. Scenario: user resolves and collapses a thread, someone replies, thread pops back open.

I notice the test at discussions_spec.js explicitly asserts expect(discussion.hidden).toBe(false) for this case - was the reset intentional? If so, what handles re-hiding the thread after a reply to a resolved discussion?

This is behind the rapid_diffs_on_mr_show feature flag + querystring opt-in, so not blocking - but should have a follow-up. Same concern applies to repliesExpanded and isReplying.

Thomas Randolph (f8d19bc0) at 16 Mar 21:55

... and 1 more commit

Thomas Randolph (28d06279) at 16 Mar 21:55

What does this MR do and why?

Previously the UI for the secrets manager was not checking nor handling if OpenBao was having issues, meaning a user would be able to proceed with actions like trying to create a secret when failure was guaranteed. Likewise on the project/group settings page, a user would try to enable or disable the secrets manager by interacting with the toggle, but with an unhealthy OpenBao instance, it would cause the toggle to enter an indefinite loading state.

The solution proposed is to use a OpenBao health query to figure out if the instance is healthy, and if it is not healthy, then make the UI reflect that in the following ways:

On the secrets manager page,

• show an error alert informing the user of the issue, and hide the action button to create a new secret on the empty state
• if a user enters the url to the new secret form directly, redirect to the main secrets page where they will encounter the above

On the project/group settings page,

• disable the secrets manager toggle
• hide the secrets permission table (it would not load values anyway, and interacting with it to try and add a new perm would fail)
• show an error alert indicating there is an issue

References

Issue/design: #582363

There are 2 other MRs currently working around this area that may conflict !226431 and !222583

Screenshots or screen recordings

SECRETS MANAGER PAGE:

Before: showed generalized error message, action button to create secret is still present

After: more specific error messaging, action button hidden

SETTINGS PAGE:

Before: all actions available even though it would fail

if a user interacted with the toggle, it would get stuck in this infinite loading state

After: permissions table hidden, error message displayed, toggle disabled

How to set up and validate locally

1. Upload a Premium license (or above).
2. Set up openbao and enable the secrets manager on your gdk.
3. Enable the secrets_manager and group_secrets_manager feature flags.
4. Go to your project/group settings (Settings > General) and expand Permissions and group features (for groups) or Visibility, project features, permissions for projects. Enable the secrets manager for your project/group with the settings toggle.
5. In your terminal, do gdk stop openbao to simulate a downed instance
6. Check the settings area from step 4 to confirm the error state is correct
7. Go to Secure > Secrets Manager and confirm the error state there

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #582363

Parameter coercion will not occur in this case. present? will interrupt and this is identical to not passing it at all.

What does this MR do?

Updates the useMergeRequestDiscussions Pinia store so that createLineDiscussion and replyToDiscussion call notes.saveNote (instead of createNewNote/replyToDiscussion directly) and pass the exact request payloads the server expects.

Problem

The existing store actions were calling notes.createNewNote for line discussions and notes.replyToDiscussion for replies with incomplete payloads — missing target_type, merge_request_diff_head_sha, position JSON with diff refs, line_type, and other required fields. The server would reject or mishandle these requests.

Additionally, saveNote in the Notes store handles placeholder notes and quick actions, which createNewNote does not. Using saveNote ensures consistent behavior with the legacy diffs code.

Changes

merge_request/stores/merge_request_discussions.js:

• createLineDiscussion — now calls notes.saveNote with a full DiffNote payload built by buildLineDiscussionData. Reads diffRefs from useMergeRequestVersions and view config from useDiffsView.
• replyToDiscussion — now calls notes.saveNote with buildReplyData payload including target_type and merge_request_diff_head_sha.
• saveNote (update) — now calls notes.updateNote with buildUpdateNoteData payload including target_type and target_id.

merge_request/utils.js (new): Pure utility functions that build the exact request payloads:

• buildLineDiscussionData — constructs the full DiffNote creation payload with position JSON containing diff refs, line info, position_type, line_range, and ignore_whitespace_change.
• buildReplyData — constructs the reply payload with in_reply_to_discussion_id, target_type, and merge_request_diff_head_sha.
• buildUpdateNoteData — constructs the update payload with target_type and target_id.
• toLineType — maps lineChange (from the DOM adapter) to 'new'/'old' line types.

These are pure functions with no store dependencies, making them easy to test in isolation.

Why?

Components need to be able to call store.createLineDiscussion(form, noteData) and have the store handle the full payload construction and API call. This keeps components simple (they just pass position, lineChange, lineCode, and note text) while the store ensures the server receives a valid DiffNote payload.

Using notes.saveNote (instead of createNewNote) is important because saveNote handles:

1. Placeholder note insertion for optimistic UI
2. Quick action parsing
3. Routing to the correct endpoint based on in_reply_to_discussion_id

Depends on

• !226952 — Exposes diff refs from server and creates the versions store

MR chain

@slashmanov there isn't really an actionable item with that non-blocking comment since it's fundamental to the data available from the BE. However, I think we should probably track that this is a delta from the legacy diffs AND it's a defect surface area that a user could (in rare circumstances) run into.

The legacy code in diffs/store/utils.js:159 has a guard for whitespace-only diff files:

ignore_whitespace_change: diffFile.whitespaceOnlyChange ? false : !showWhitespace

When a user has "hide whitespace" enabled and comments on a line in a whitespace-only file, the server needs whitespace visible to resolve the note's position -- otherwise the diff collapses entirely and the line becomes unresolvable. The legacy code forces ignore_whitespace_change to false in that case. Here, it's always !viewConfig.showWhitespace, which would send true and could result in a misplaced note or server error.

Having traced this through, the whitespace_only? flag exists on the server-side diff_file object (e.g. no_preview_component.rb:65) but is never serialized into the file_data JSON that the <diff-file> web component receives (DiffFileComponent#file_data), so this guard can't be replicated here today without a server-side change to expose that flag.

This is a narrow edge case, but it's worth tracking as a known behavioral regression in Rapid Diffs relative to the legacy path -- the kind of gap that's easy to lose sight of if it isn't written down somewhere.

Deployed to production

Hey @marcel.amirault since this was applied, I'm going to bypass your change request to potentially unblock merge.