Remove help panel tabs in favor of static sections#309
Remove help panel tabs in favor of static sections#309apinkert wants to merge 5 commits intoRedHatInsights:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughHelp Panel rewritten from a dynamic, multi-tier/custom-tab model to a static, single-tier main-tab layout filtered by feature flags. Add/close/subtab mechanics removed; Quick Starts render in a full-panel overlay. Tests, stories, styles, MSW mocks, and docs updated to match the new model. ChangesHelp Panel Single-Tier Architecture
Sequence DiagramsequenceDiagram
actor User
participant Tabs as Main Tabs
participant State as activeTabId State
participant Content as HelpPanelTabContainer
participant QS as QuickStartsPanel (Overlay)
User->>Tabs: Click tab (Search / Learn / APIs / Support / Feedback / VA)
Tabs->>State: set activeTabId
State->>Content: render content for activeTabId
Content->>Content: display selected panel
rect rgba(100, 150, 200, 0.5)
Note over User,QS: Quick Start Overlay Flow
User->>Content: trigger quickstart open
Content->>QS: set openQuickstartInHelpPanelStore.pending = quickstartId
QS->>QS: render full QuickStartsPanel overlay
User->>QS: close overlay
QS->>State: restore previous activeTabId
State->>Content: re-render previous panel
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cypress/component/HelpPanel.cy.tsx (1)
60-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInitialize Scalprum once per mount, not on every render.
useRef(initialize(...))evaluates theinitialize()call on every render, even though it only uses the value from the initial mount. When the component re-renders aftersetIsReady(true)in theuseEffect,initialize()runs again, mutating global Scalprum state and potentially leaking test registrations across assertions.Per React documentation, the correct pattern for lazy initialization of a side-effectful object is to use
useRef(null)with a conditional check, notuseState(() => ...)oruseMemo:♻️ Proposed fix
- const scalprum = useRef( - initialize({ - appsConfig: { - virtualAssistant: { - name: 'virtualAssistant', - manifestLocation: '/foo/bar.json', - }, - }, - api: api || defaultApi, - }) - ); + const scalprum = useRef(null); + if (scalprum.current === null) { + scalprum.current = initialize({ + appsConfig: { + virtualAssistant: { + name: 'virtualAssistant', + manifestLocation: '/foo/bar.json', + }, + }, + api: api || defaultApi, + }); + }🤖 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 `@cypress/component/HelpPanel.cy.tsx` around lines 60 - 69, The current useRef(initialize(...)) calls initialize on every render; change to lazy-init by creating scalprum with useRef(null) and only call initialize(...) once (e.g., if (!scalprum.current) scalprum.current = initialize({...})) so initialize() is executed a single time per mount; update any places that read scalprum to use scalprum.current and ensure this check occurs before/inside the existing useEffect that calls setIsReady(true) to avoid re-running initialize and leaking registrations.
🧹 Nitpick comments (2)
src/components/HelpPanel/HelpPanelCustomTabs.test.tsx (1)
291-315: ⚡ Quick winAssert that the deprecated ref API is a no-op.
This only checks
console.warn, so it still passes ifopenTabWithContentwarns and mutates tab state. Please also assert that no extra tab or custom content becomes visible after the call.♻️ Suggested assertion additions
ref.current?.openTabWithContent({ id: 'custom-tab', title: 'Custom Content', tabType: TabType.learn, content: <div>Custom content here</div>, }); expect(consoleWarnSpy).toHaveBeenCalledWith( expect.stringContaining( 'openTabWithContent is no longer supported with static tabs' ) ); + expect( + screen.queryByRole('tab', { name: /custom content/i }) + ).not.toBeInTheDocument(); + expect(screen.queryByText('Custom content here')).not.toBeInTheDocument(); consoleWarnSpy.mockRestore();🤖 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 `@src/components/HelpPanel/HelpPanelCustomTabs.test.tsx` around lines 291 - 315, The test currently only checks console.warn but must also assert the deprecated openTabWithContent is a no-op; before calling ref.current?.openTabWithContent, capture the current visible tab state (e.g., count of rendered tabs via getAllByRole('tab') or presence of a known static tab), call HelpPanelCustomTabsRef.openTabWithContent, then assert no new tab or content appears by verifying that screen.queryByText('Custom content here') is null and that the tab count (or presence of the known static tab) is unchanged; reference the HelpPanelCustomTabs component, the HelpPanelCustomTabsRef.openTabWithContent call, and TabType.learn when adding these assertions.cypress/component/HelpPanel.cy.tsx (1)
127-141: ⚡ Quick winThis feature-flag test currently looks like a false positive.
The assertion only checks that
knowledgeBaseTitleis absent, but the static-tab UI in this PR no longer exposes Knowledge Base as a visible main tab. That means this test can keep passing even if the KB flag stops being consulted entirely. Please assert against the actual flagged surface, or add a control case proving the enabled state is visible first.🤖 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 `@cypress/component/HelpPanel.cy.tsx` around lines 127 - 141, The test "should not display tabs hidden by FF" is a false positive because it only asserts absence of getMessageText('knowledgeBaseTitle') while the PR removed Knowledge Base as a main tab; update the test to assert the actual flagged surface by (a) adding a control case that mounts <HelpPanel toggleDrawer={toggleDrawerSpy} /> with the KB flag enabled (use defaultFlags or a copy with enabled: true) and assert the KB UI element is visible, and (b) keep the existing mount with disabledFlags (copying defaultFlags[0] with enabled: false) and assert the exact KB-specific element used by HelpPanel is not present; reference HelpPanel, Wrapper, defaultFlags, and getMessageText('knowledgeBaseTitle') to locate and update the assertions.
🤖 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 `@src/components/HelpPanel/HelpPanelCustomTabs.scss`:
- Around line 63-79: Current CSS hides horizontal overflow and PatternFly's
scroll/overflow controls making tabs unreachable when they exceed width; update
.lr-c-help-panel-custom-tabs rules so the tab list allows horizontal scrolling
(change overflow-x: hidden to overflow-x: auto or remove it), and remove the
display:none !important rules for .pf-v6-c-tabs__scroll-button and
.pf-v6-c-tabs__item.pf-m-overflow so PatternFly's navigation controls can
appear; alternatively, implement a responsive layout (e.g., wrapping or a
compact variant) in the same selector if you prefer tabs to reflow instead of
scrolling.
In `@src/components/HelpPanel/HelpPanelCustomTabs.tsx`:
- Around line 196-202: openTabWithContent currently only logs a warning while
HelpPanelContent (and in-page HelpPanelLink flows) still call it with a newTab
payload, so clicks open the drawer but ignore the requested section; implement a
real translation here: accept the HelpPanelTabContent payload in
openTabWithContent, map its identifier/section to the existing static tab id or
index (translate newTab -> static-section model), then programmatically invoke
the same tab-selection flow used by the static tabs (e.g., call the existing tab
click handler / setActiveTab / open drawer method) so the requested section
displays; alternatively, if you prefer not to support the payload, remove/update
callers (HelpPanelContent / HelpPanelLink) in the same PR instead of leaving a
no-op.
In `@src/docs/ComponentReference.mdx`:
- Around line 526-533: Docs still describe removed dynamic-tab APIs (newTab,
openTabWithContent, HelpPanelLink custom tabs) while the implementation now uses
static memoized lists (allTabs, tabs). Update the earlier sections to remove or
mark as deprecated the dynamic-tab guidance and replace it with the new pattern:
explain that tabs are created via createMainTabs(showVA) and filtered with
filterTabsByFeatureFlags(allTabs, flags), show how to use the allTabs/tabs
memoization pattern, and add a short migration note pointing consumers from
newTab/openTabWithContent/HelpPanelLink to the new static approach.
In `@src/user-journeys/HelpPanelSearchPanel.stories.tsx`:
- Around line 139-145: The current assertion uses canvas.findAllByText and the
results variable which matches any title anywhere in the canvas (e.g.,
recommended content), so scope the query to the actual search-results list: use
canvas.getByRole('list', { name: /search results/i }) (or canvas.getByLabelText
if that’s how it’s exposed) and run within(thatList).findAllByText('Getting
started with Red Hat Insights', {}, { timeout: 5000 }) then assert on the
length; update the three similar blocks (the current canvas.findAllByText
occurrences and the ones around the other mentioned ranges) to target the list
instead of the whole canvas.
In `@src/user-journeys/HelpPanelSupportPanel.stories.tsx`:
- Around line 125-129: The test currently uses within(document.body) and
getAllByText to find "open a support case", which can match duplicates anywhere;
scope the query to the support empty-state container instead: locate the
help-panel-support-empty-state element (e.g., via getByTestId or getByRole for
the empty-state container), create a scoped within(...) for that element instead
of documentBody, then call getAllByText(/open a support case/i) on that scoped
query (openButtons) and assert its length; replace references to
within(document.body) and openButtons lookup accordingly.
---
Outside diff comments:
In `@cypress/component/HelpPanel.cy.tsx`:
- Around line 60-69: The current useRef(initialize(...)) calls initialize on
every render; change to lazy-init by creating scalprum with useRef(null) and
only call initialize(...) once (e.g., if (!scalprum.current) scalprum.current =
initialize({...})) so initialize() is executed a single time per mount; update
any places that read scalprum to use scalprum.current and ensure this check
occurs before/inside the existing useEffect that calls setIsReady(true) to avoid
re-running initialize and leaking registrations.
---
Nitpick comments:
In `@cypress/component/HelpPanel.cy.tsx`:
- Around line 127-141: The test "should not display tabs hidden by FF" is a
false positive because it only asserts absence of
getMessageText('knowledgeBaseTitle') while the PR removed Knowledge Base as a
main tab; update the test to assert the actual flagged surface by (a) adding a
control case that mounts <HelpPanel toggleDrawer={toggleDrawerSpy} /> with the
KB flag enabled (use defaultFlags or a copy with enabled: true) and assert the
KB UI element is visible, and (b) keep the existing mount with disabledFlags
(copying defaultFlags[0] with enabled: false) and assert the exact KB-specific
element used by HelpPanel is not present; reference HelpPanel, Wrapper,
defaultFlags, and getMessageText('knowledgeBaseTitle') to locate and update the
assertions.
In `@src/components/HelpPanel/HelpPanelCustomTabs.test.tsx`:
- Around line 291-315: The test currently only checks console.warn but must also
assert the deprecated openTabWithContent is a no-op; before calling
ref.current?.openTabWithContent, capture the current visible tab state (e.g.,
count of rendered tabs via getAllByRole('tab') or presence of a known static
tab), call HelpPanelCustomTabsRef.openTabWithContent, then assert no new tab or
content appears by verifying that screen.queryByText('Custom content here') is
null and that the tab count (or presence of the known static tab) is unchanged;
reference the HelpPanelCustomTabs component, the
HelpPanelCustomTabsRef.openTabWithContent call, and TabType.learn when adding
these assertions.
🪄 Autofix (Beta)
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: Enterprise
Run ID: acdb829b-4826-477e-98b3-df37993e5715
📒 Files selected for processing (14)
cypress/component/HelpPanel.cy.tsxsrc/components/HelpPanel/HelpPanelContent.stories.tsxsrc/components/HelpPanel/HelpPanelContent.tsxsrc/components/HelpPanel/HelpPanelCustomTabs.scsssrc/components/HelpPanel/HelpPanelCustomTabs.test.tsxsrc/components/HelpPanel/HelpPanelCustomTabs.tsxsrc/docs/ComponentReference.mdxsrc/docs/HelpPanelArchitecture.mdxsrc/user-journeys/HelpPanelAPIPanel.stories.tsxsrc/user-journeys/HelpPanelInPageLinks.stories.tsxsrc/user-journeys/HelpPanelKBPanel.stories.tsxsrc/user-journeys/HelpPanelSearchPanel.stories.tsxsrc/user-journeys/HelpPanelSupportPanel.stories.tsxsrc/user-journeys/_shared/helpPanelJourneyHelpers.ts
💤 Files with no reviewable changes (1)
- src/user-journeys/HelpPanelKBPanel.stories.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/HelpPanel/HelpPanelCustomTabs.test.tsx (1)
168-179:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the chatbot flag mocks in sync.
This test flips
platform.chrome.help-panel_chatbottofalseviamockUseFlag, butmockUseFlagsstill reports that flag as enabled frombeforeEach. That gives the component conflicting inputs and can let the assertion pass for the wrong reason. OverridemockUseFlagshere as well so both hooks describe the same state.Suggested fix
mockUseFlag.mockImplementation((flagName: string) => { if (flagName === 'platform.chrome.help-panel_chatbot') return false; return true; }); + mockUseFlags.mockReturnValue([ + { name: 'platform.chrome.help-panel_search', enabled: true }, + { name: 'platform.chrome.help-panel_knowledge-base', enabled: true }, + { name: 'platform.chrome.help-panel_chatbot', enabled: false }, + ]); renderWithIntl(<HelpPanelCustomTabs />);🤖 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 `@src/components/HelpPanel/HelpPanelCustomTabs.test.tsx` around lines 168 - 179, The test toggles the chatbot flag via mockUseFlag but leaves mockUseFlags returning the original enabled value from beforeEach, causing inconsistent inputs; update this test to also override mockUseFlags to return the same false state for 'platform.chrome.help-panel_chatbot' before rendering HelpPanelCustomTabs so both hooks agree (use mockUseFlags.mockImplementation or similar to check flagName and return false for the chatbot flag and true otherwise), then renderWithIntl and assert vaTab is not in the document.src/components/HelpPanel/HelpPanelCustomTabs.scss (1)
100-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAnchor the quickstart overlay to the tab-content container.
The new overlay is absolutely positioned, but this stylesheet never establishes a local containing block for it. As written,
top/right/bottom/left: 0can resolve against an ancestor outside the tab body, which can make the quickstart view cover the drawer header/close button instead of just the content area.Suggested fix
.lr-c-help-panel-tab-content--quickstart { display: flex; flex-direction: column; min-height: 0; height: 100%; + position: relative; } @@ .lr-c-help-panel-quickstart-overlay { position: absolute; - top: 0; - left: 0; - right: 0; - bottom: 0; + inset: 0; background-color: var(--pf-t--global--background--color--primary--default); z-index: 100; display: flex;Also applies to: 145-158
🤖 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 `@src/components/HelpPanel/HelpPanelCustomTabs.scss` around lines 100 - 105, The quickstart overlay is absolutely positioned but its container (.lr-c-help-panel-tab-content--quickstart) does not establish a containing block, so top/right/bottom/left can resolve against ancestors and let the overlay cover the header/close button; fix by making the tab-content container a positioned containing block (e.g. add position: relative) so the overlay is anchored to .lr-c-help-panel-tab-content--quickstart (apply same change to the other related tab-content selector mentioned in the comment).
🧹 Nitpick comments (1)
src/user-journeys/_shared/helpPanelJourneyHelpers.ts (1)
506-512: ⚡ Quick winDeduplicate Support Cases handlers to prevent mock drift
These two handlers duplicate the same prod/stage endpoints and response shape already defined in
supportPanelMswHandlers. Keeping both copies can drift over time and cause inconsistent Storybook behavior across journeys. Consider extracting one shared handler set and reusing it in both arrays.🤖 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 `@src/user-journeys/_shared/helpPanelJourneyHelpers.ts` around lines 506 - 512, The two http.post handlers returning HttpResponse.json({ cases: [] }) for 'https://api.access.redhat.com/support/v1/cases/filter' and the stage URL are duplicates of the handlers in supportPanelMswHandlers; remove these local duplicates from helpPanelJourneyHelpers and instead import/reuse the shared supportPanelMswHandlers (or extract a single shared handler constant, e.g., supportCasesHandler) so both handler arrays reference the same handler object to prevent mock drift.
🤖 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 `@cypress/component/HelpPanel.cy.tsx`:
- Around line 132-138: The test currently builds disabledSearchFlag by replacing
defaultFlags[1], which drops all other flags; instead keep the original
defaultFlags array and override only the Search flag key
platform.chrome.help-panel_search to false. Update the fixture to start from
defaultFlags, create a shallow copy (e.g., [...defaultFlags]) and modify the
entry whose key is "platform.chrome.help-panel_search" (or map over defaultFlags
and set enabled: false when flag.key === "platform.chrome.help-panel_search"),
then pass that flags array into Wrapper so only Search is disabled while all
other default flags remain unchanged.
---
Outside diff comments:
In `@src/components/HelpPanel/HelpPanelCustomTabs.scss`:
- Around line 100-105: The quickstart overlay is absolutely positioned but its
container (.lr-c-help-panel-tab-content--quickstart) does not establish a
containing block, so top/right/bottom/left can resolve against ancestors and let
the overlay cover the header/close button; fix by making the tab-content
container a positioned containing block (e.g. add position: relative) so the
overlay is anchored to .lr-c-help-panel-tab-content--quickstart (apply same
change to the other related tab-content selector mentioned in the comment).
In `@src/components/HelpPanel/HelpPanelCustomTabs.test.tsx`:
- Around line 168-179: The test toggles the chatbot flag via mockUseFlag but
leaves mockUseFlags returning the original enabled value from beforeEach,
causing inconsistent inputs; update this test to also override mockUseFlags to
return the same false state for 'platform.chrome.help-panel_chatbot' before
rendering HelpPanelCustomTabs so both hooks agree (use
mockUseFlags.mockImplementation or similar to check flagName and return false
for the chatbot flag and true otherwise), then renderWithIntl and assert vaTab
is not in the document.
---
Nitpick comments:
In `@src/user-journeys/_shared/helpPanelJourneyHelpers.ts`:
- Around line 506-512: The two http.post handlers returning HttpResponse.json({
cases: [] }) for 'https://api.access.redhat.com/support/v1/cases/filter' and the
stage URL are duplicates of the handlers in supportPanelMswHandlers; remove
these local duplicates from helpPanelJourneyHelpers and instead import/reuse the
shared supportPanelMswHandlers (or extract a single shared handler constant,
e.g., supportCasesHandler) so both handler arrays reference the same handler
object to prevent mock drift.
🪄 Autofix (Beta)
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: Enterprise
Run ID: b8e109f4-9a52-4c43-bc1b-a0457cd88a9e
📒 Files selected for processing (8)
cypress/component/HelpPanel.cy.tsxsrc/components/HelpPanel/HelpPanelCustomTabs.scsssrc/components/HelpPanel/HelpPanelCustomTabs.test.tsxsrc/components/HelpPanel/HelpPanelCustomTabs.tsxsrc/docs/ComponentReference.mdxsrc/user-journeys/HelpPanelSearchPanel.stories.tsxsrc/user-journeys/HelpPanelSupportPanel.stories.tsxsrc/user-journeys/_shared/helpPanelJourneyHelpers.ts
✅ Files skipped from review due to trivial changes (1)
- src/docs/ComponentReference.mdx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/user-journeys/HelpPanelSearchPanel.stories.tsx
- src/components/HelpPanel/HelpPanelCustomTabs.tsx
- src/user-journeys/HelpPanelSupportPanel.stories.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/components/HelpPanel/HelpPanelTabs/SupportPanel.stories.tsx`:
- Line 132: Prettier flagged the assertion line using canvas.getAllByText; fix
it by reformatting the statement to match project Prettier rules (run Prettier
or your editor formatter) — locate the assertion using expect(...) and
canvas.getAllByText('Waiting on Customer').length and ensure the line ends with
the properly styled semicolon and quote style consistent with the repo so the
linter stops reporting prettier/prettier.
🪄 Autofix (Beta)
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: Enterprise
Run ID: 4649123a-7bc8-400c-a667-c922f3c1ea4e
📒 Files selected for processing (4)
src/components/HelpPanel/HelpPanelContent.stories.tsxsrc/components/HelpPanel/HelpPanelTabs/SupportPanel.stories.tsxsrc/user-journeys/HelpPanelSupportPanel.stories.tsxsrc/user-journeys/_shared/helpPanelJourneyHelpers.ts
✅ Files skipped from review due to trivial changes (1)
- src/user-journeys/HelpPanelSupportPanel.stories.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/HelpPanel/HelpPanelContent.stories.tsx
- src/user-journeys/_shared/helpPanelJourneyHelpers.ts
For RHCLOUD-47505.
Removes the tab structure from the help panel (along with the ability to open new tabs) and replaces it with static sections for each panel.
Screen.Recording.2026-05-07.at.5.10.02.PM.mov