Skip to content

PMM-15302 Remove ssh settings tab in HA mode - #5744

Open
matejkubinec wants to merge 2 commits into
mainfrom
PMM-15302-ha-remove-ssh-settings
Open

PMM-15302 Remove ssh settings tab in HA mode#5744
matejkubinec wants to merge 2 commits into
mainfrom
PMM-15302-ha-remove-ssh-settings

Conversation

@matejkubinec

@matejkubinec matejkubinec commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Ticket number: PMM-15302

Feature build: SUBMODULES-4515

@matejkubinec
matejkubinec marked this pull request as ready for review August 7, 2026 13:36
@matejkubinec
matejkubinec requested a review from a team as a code owner August 7, 2026 13:36
@matejkubinec
matejkubinec requested review from fabio-silva and mattiasimonato and removed request for a team August 7, 2026 13:36
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Changes

HA-aware SSH settings access

Layer / File(s) Summary
Settings tab HA gate
ui/apps/pmm/src/pages/settings/Settings.tsx
Settings retrieves HA status and renders the SSH key tab only when the status is Disabled.
SSH form guard and validation
ui/apps/pmm/src/pages/settings/components/ssh-key/SshKeyForm.tsx, ui/apps/pmm/src/pages/settings/Settings.test.tsx
SshKeyForm redirects to /settings when HA is enabled. Tests mock the HA API, set the default status to disabled, and verify that the SSH tab is hidden when HA is enabled.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes removing the SSH settings tab when PMM runs in HA mode.
Description check ✅ Passed The description includes the required ticket number and feature build, and the API documentation section is not applicable because no API changes are described.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
ui/apps/pmm/src/pages/settings/components/ssh-key/SshKeyForm.tsx (1)

49-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the project route constant.

Navigate hardcodes "/settings" at Line 50. Use the settings-route constant from ui/apps/pmm/src/lib/constants.ts instead of embedding this path.

As per coding guidelines, use constants from ui/apps/pmm/src/lib/constants.ts instead of embedding URL strings.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/apps/pmm/src/pages/settings/components/ssh-key/SshKeyForm.tsx` around
lines 49 - 50, Update the redirect in SshKeyForm’s haStatus handling to use the
existing settings-route constant from constants.ts instead of the hardcoded
"/settings" path, preserving the current Navigate behavior.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@ui/apps/pmm/src/pages/settings/components/ssh-key/SshKeyForm.tsx`:
- Around line 49-52: Make HA enforcement fail closed across the SSH settings
route: in ui/apps/pmm/src/pages/settings/components/ssh-key/SshKeyForm.tsx lines
15-24, expose the HA query readiness and error state, and in lines 49-52 allow
the form only for confirmed Disabled status while blocking or redirecting
pending, error, and Enabled states. In
ui/apps/pmm/src/pages/settings/Settings.tsx lines 29 and 70-76, propagate
readiness to the parent and gate or redirect direct SSH-route content rather
than merely hiding the tab. In ui/apps/pmm/src/pages/settings/Settings.test.tsx
lines 97-103, add a co-located direct-route test without mocking SshKeyForm that
verifies unknown HA status cannot render the form.

---

Nitpick comments:
In `@ui/apps/pmm/src/pages/settings/components/ssh-key/SshKeyForm.tsx`:
- Around line 49-50: Update the redirect in SshKeyForm’s haStatus handling to
use the existing settings-route constant from constants.ts instead of the
hardcoded "/settings" path, preserving the current Navigate behavior.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 1d4f2c2e-2827-4c3f-b0da-62270b39c51a

📥 Commits

Reviewing files that changed from the base of the PR and between 0415b33 and a1df4f9.

📒 Files selected for processing (3)
  • ui/apps/pmm/src/pages/settings/Settings.test.tsx
  • ui/apps/pmm/src/pages/settings/Settings.tsx
  • ui/apps/pmm/src/pages/settings/components/ssh-key/SshKeyForm.tsx
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • percona/pmm-qa (manual)
  • percona/pmm (manual)

Comment on lines +49 to +52
if (haStatus?.status === 'Enabled') {
return <Navigate to="/settings" />;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Make HA enforcement fail closed for direct SSH routes.

The parent and child checks do not make one fail-closed decision. The tab disappears, but a direct route can still render and submit the form while HA status is unknown.

  • ui/apps/pmm/src/pages/settings/components/ssh-key/SshKeyForm.tsx#L49-L52: allow the form only after a confirmed Disabled response; block or redirect pending and error states.
  • ui/apps/pmm/src/pages/settings/components/ssh-key/SshKeyForm.tsx#L15-L24: expose the query readiness and error state required by the guard.
  • ui/apps/pmm/src/pages/settings/Settings.tsx#L29-L29: keep the parent aware of HA-query readiness for direct-route handling.
  • ui/apps/pmm/src/pages/settings/Settings.tsx#L70-L76: gate the SSH content or redirect /settings/ssh-key, not only remove the tab.
  • ui/apps/pmm/src/pages/settings/Settings.test.tsx#L97-L103: add a direct-route assertion without the SshKeyForm mock, or add it in a co-located form test.

As per coding guidelines, co-locate Vitest tests with the UI code they cover.

📍 Affects 3 files
  • ui/apps/pmm/src/pages/settings/components/ssh-key/SshKeyForm.tsx#L49-L52 (this comment)
  • ui/apps/pmm/src/pages/settings/components/ssh-key/SshKeyForm.tsx#L15-L24
  • ui/apps/pmm/src/pages/settings/Settings.tsx#L29-L29
  • ui/apps/pmm/src/pages/settings/Settings.tsx#L70-L76
  • ui/apps/pmm/src/pages/settings/Settings.test.tsx#L97-L103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/apps/pmm/src/pages/settings/components/ssh-key/SshKeyForm.tsx` around
lines 49 - 52, Make HA enforcement fail closed across the SSH settings route: in
ui/apps/pmm/src/pages/settings/components/ssh-key/SshKeyForm.tsx lines 15-24,
expose the HA query readiness and error state, and in lines 49-52 allow the form
only for confirmed Disabled status while blocking or redirecting pending, error,
and Enabled states. In ui/apps/pmm/src/pages/settings/Settings.tsx lines 29 and
70-76, propagate readiness to the parent and gate or redirect direct SSH-route
content rather than merely hiding the tab. In
ui/apps/pmm/src/pages/settings/Settings.test.tsx lines 97-103, add a co-located
direct-route test without mocking SshKeyForm that verifies unknown HA status
cannot render the form.

Source: Coding guidelines

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant