feat: move status link to match mocks and add tab stories#266
Merged
apinkert merged 4 commits intoRedHatInsights:masterfrom Mar 16, 2026
Merged
feat: move status link to match mocks and add tab stories#266apinkert merged 4 commits intoRedHatInsights:masterfrom
apinkert merged 4 commits intoRedHatInsights:masterfrom
Conversation
Summary by CodeRabbit
WalkthroughRemoves virtual-assistant and feature-flag logic from the Help Panel, narrows the public API by removing two optional props and exported types, makes the status page link always render with updated label/styling, consolidates Playwright tests, and adds Storybook stories for Help Panel behaviors. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/docs/ComponentReference.mdx (1)
53-55: Clarify the provider assumptions around this example.This snippet now looks self-contained, but
HelpPanelContentstill depends on ambient shell/providers from the app tree. A short note here, or pointing readers at the wrapper pattern used insrc/components/HelpPanel/HelpPanelContent.stories.tsx, would keep this from being copied into an environment where it fails immediately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/docs/ComponentReference.mdx` around lines 53 - 55, The example using HelpPanelContent is missing a note that it depends on ambient app providers; update the docs around the <HelpPanelContent toggleDrawer={handleCloseDrawer} /> snippet to state that HelpPanelContent expects the surrounding shell/providers (e.g., theme, context, router) and will fail if rendered standalone, and add a link or pointer to the wrapper used in src/components/HelpPanel/HelpPanelContent.stories.tsx as the recommended pattern for embedding it (or show the wrapper usage) so readers know to wrap HelpPanelContent with the same providers before copying this example.src/components/HelpPanel/HelpPanelContent.stories.tsx (1)
169-185: Prefer the stable test hook here over the CSS class.The component now exposes a dedicated OUIA locator for this link, but the story still queries
.lr-c-status-page-link. That couples the interaction test to styling, so a harmless restyle can break the story without any behavior change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/HelpPanel/HelpPanelContent.stories.tsx` around lines 169 - 185, Replace the fragile CSS selector in the waitFor block that finds statusPageLink (currently document.querySelector('.lr-c-status-page-link')) with the component's OUIA locator; update the query to use the OUIA attribute selector (for example query for '[data-ouia-component-id="status-page-link"]' or the project’s specific OUIA attribute) so waitFor, the statusPageLink variable and subsequent expects use the stable locator rather than the '.lr-c-status-page-link' class.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/HelpPanel/HelpPanelContent.stories.tsx`:
- Around line 324-339: The test currently lets the assertion pass when
visibleNewTabs.length >= 1, which doesn't prove overflow occurred; update the
test that uses addTabButton and waitFor so it repeatedly clicks addTabButton (or
programmatically adds tabs) until the overflow trigger appears, then assert
directly that document.querySelector('.pf-v6-c-tabs__item.pf-m-overflow')
(overflowItem) is truthy; alternatively set a narrower viewport/container width
before adding tabs to force overflow, then assert on overflowItem rather than
relying on visibleNewTabs.
In `@src/components/HelpPanel/HelpPanelContent.tsx`:
- Around line 41-53: The external link Button in HelpPanelContent (the Button
with className "lr-c-status-page-link" and data-ouia-component-id
"help-panel-status-page-header-button") opens with target="_blank" but is
missing a rel attribute; update that Button's props to include rel="noopener
noreferrer" so the new tab cannot retain window.opener (matching the project's
external-link hardening pattern).
In `@src/components/HelpPanel/HelpPanelCustomTabs.scss`:
- Around line 132-136: The .lr-c-status-page-link rule uses margin-left which
breaks RTL support; replace margin-left: var(--pf-t--global--spacer--md)
!important with margin-inline-start: var(--pf-t--global--spacer--md) !important
in the .lr-c-status-page-link selector so the header link uses logical start
margin and preserves RTL/LTR behavior (keep the existing font-size, font-weight,
text-decoration, and !important flags).
---
Nitpick comments:
In `@src/components/HelpPanel/HelpPanelContent.stories.tsx`:
- Around line 169-185: Replace the fragile CSS selector in the waitFor block
that finds statusPageLink (currently
document.querySelector('.lr-c-status-page-link')) with the component's OUIA
locator; update the query to use the OUIA attribute selector (for example query
for '[data-ouia-component-id="status-page-link"]' or the project’s specific OUIA
attribute) so waitFor, the statusPageLink variable and subsequent expects use
the stable locator rather than the '.lr-c-status-page-link' class.
In `@src/docs/ComponentReference.mdx`:
- Around line 53-55: The example using HelpPanelContent is missing a note that
it depends on ambient app providers; update the docs around the
<HelpPanelContent toggleDrawer={handleCloseDrawer} /> snippet to state that
HelpPanelContent expects the surrounding shell/providers (e.g., theme, context,
router) and will fail if rendered standalone, and add a link or pointer to the
wrapper used in src/components/HelpPanel/HelpPanelContent.stories.tsx as the
recommended pattern for embedding it (or show the wrapper usage) so readers know
to wrap HelpPanelContent with the same providers before copying this example.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab32a902-d519-459d-bf9b-ed479041dde1
📒 Files selected for processing (7)
playwright/help-panel.spec.tssrc/Messages.tssrc/components/HelpPanel/HelpPanelContent.stories.tsxsrc/components/HelpPanel/HelpPanelContent.tsxsrc/components/HelpPanel/HelpPanelCustomTabs.scsssrc/components/HelpPanel/HelpPanelCustomTabs.tsxsrc/docs/ComponentReference.mdx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/HelpPanel/HelpPanelContent.stories.tsx (1)
138-167: Extract repeated drawer-open setup into a shared helper.The same interaction/wait sequence is duplicated across multiple
playfunctions; extracting it will reduce drift and simplify future changes.♻️ Suggested refactor
+const openHelpPanelAndWaitReady = async ( + canvas: ReturnType<typeof within> +) => { + const helpButton = await canvas.findByLabelText('Toggle help panel', {}, { timeout: 5000 }); + expect(helpButton).toBeInTheDocument(); + await userEvent.click(helpButton); + + await waitFor(() => { + const drawer = document.querySelector('.pf-v6-c-drawer.pf-m-expanded'); + expect(drawer).toBeInTheDocument(); + }, { timeout: 5000 }); + + await waitFor(() => { + const helpPanelTitle = document.querySelector('[data-ouia-component-id="help-panel-title"]'); + expect(helpPanelTitle).toBeInTheDocument(); + }, { timeout: 5000 }); +};- // Wait for page to load and find the Help button in the header - const helpButton = await canvas.findByLabelText( - 'Toggle help panel', - {}, - { timeout: 5000 } - ); - expect(helpButton).toBeInTheDocument(); - await userEvent.click(helpButton); - await waitFor(/* drawer */); - await waitFor(/* title */); + await openHelpPanelAndWaitReady(canvas);Also applies to: 199-228, 281-310, 377-406
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/HelpPanel/HelpPanelContent.stories.tsx` around lines 138 - 167, Extract the repeated "open help panel" interaction/wait sequence into a shared helper (e.g., openHelpPanel or openHelpPanelInCanvas) and replace the duplicated blocks in each play function with a single call; the helper should accept the testing utilities/context (canvas) and perform: findByLabelText('Toggle help panel'), userEvent.click(helpButton), waitFor the drawer selector '.pf-v6-c-drawer.pf-m-expanded', and waitFor the help panel title selector '[data-ouia-component-id="help-panel-title"]' so all play functions (the blocks around the occurrences shown and at lines 199-228, 281-310, 377-406) call this helper instead of repeating the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/HelpPanel/HelpPanelContent.stories.tsx`:
- Around line 312-316: Add an explicit precondition check that the queried
element exists before the loop: after const addTabButton =
document.querySelector(...) as HTMLElement, verify addTabButton is not null
(e.g., throw or assert/expect with a clear message) so tests fail immediately
and clearly if the selector changes; update the HelpPanelContent story to
perform this single null-check on addTabButton before using it in the loop.
---
Nitpick comments:
In `@src/components/HelpPanel/HelpPanelContent.stories.tsx`:
- Around line 138-167: Extract the repeated "open help panel" interaction/wait
sequence into a shared helper (e.g., openHelpPanel or openHelpPanelInCanvas) and
replace the duplicated blocks in each play function with a single call; the
helper should accept the testing utilities/context (canvas) and perform:
findByLabelText('Toggle help panel'), userEvent.click(helpButton), waitFor the
drawer selector '.pf-v6-c-drawer.pf-m-expanded', and waitFor the help panel
title selector '[data-ouia-component-id="help-panel-title"]' so all play
functions (the blocks around the occurrences shown and at lines 199-228,
281-310, 377-406) call this helper instead of repeating the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba90f9c5-24fd-42bd-8179-57bb4e5019ff
📒 Files selected for processing (2)
src/components/HelpPanel/HelpPanelContent.stories.tsxsrc/components/HelpPanel/HelpPanelContent.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/components/HelpPanel/HelpPanelContent.stories.tsx (1)
312-316:⚠️ Potential issue | 🟡 MinorAdd an explicit precondition assertion for
addTabButton.Unlike
OpenAndCloseTabs(line 234), this story doesn't assert thataddTabButtonexists before entering the loop. If the selector changes, failures would be unclear.✅ Proposed fix
const addTabButton = document.querySelector( '[data-ouia-component-id="help-panel-add-tab-button"]' ) as HTMLElement; + expect(addTabButton).toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/HelpPanel/HelpPanelContent.stories.tsx` around lines 312 - 316, The story queries addTabButton via document.querySelector but doesn't assert it exists before use; add an explicit precondition check (e.g., throw or use expect) immediately after obtaining addTabButton to ensure the element is non-null before entering the loop in this story (mirroring the OpenAndCloseTabs approach), referencing the addTabButton variable so failures show a clear message if the selector changes.
🧹 Nitpick comments (2)
src/components/HelpPanel/HelpPanelContent.stories.tsx (2)
349-368: Conditional logic could silently pass when expected behavior fails.If
visibleNewTabs.lengthis 0 orcloseButtonis null, the test completes without actually testing the tab close functionality. Consider asserting these preconditions or restructuring to ensure the close operation is always exercised.♻️ Suggested approach
const visibleNewTabs = canvas.queryAllByRole('tab', { name: 'New tab' }); + expect(visibleNewTabs.length).toBeGreaterThan(0); - if (visibleNewTabs.length > 0) { - // Close the first visible new tab - const firstTabItem = visibleNewTabs[0].closest('.pf-v6-c-tabs__item'); - const closeButton = firstTabItem?.querySelector( - '.pf-v6-c-tabs__item-action button' - ) as HTMLElement; + // Close the first visible new tab + const firstTabItem = visibleNewTabs[0].closest('.pf-v6-c-tabs__item'); + const closeButton = firstTabItem?.querySelector( + '.pf-v6-c-tabs__item-action button' + ) as HTMLElement; + expect(closeButton).toBeInTheDocument(); - if (closeButton) { - await userEvent.click(closeButton); + await userEvent.click(closeButton); - // Wait a bit for the tab to close - await waitFor( - () => { - const tabs = canvas.queryAllByRole('tab', { name: 'New tab' }); - expect(tabs.length).toBeLessThan(visibleNewTabs.length); - }, - { timeout: 3000 } - ); - } - } + // Wait a bit for the tab to close + await waitFor( + () => { + const tabs = canvas.queryAllByRole('tab', { name: 'New tab' }); + expect(tabs.length).toBeLessThan(visibleNewTabs.length); + }, + { timeout: 3000 } + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/HelpPanel/HelpPanelContent.stories.tsx` around lines 349 - 368, The test currently skips exercising the close path when visibleNewTabs is empty or closeButton is null; update the block around visibleNewTabs/firstTabItem/closeButton so it asserts preconditions before proceeding (e.g., expect(visibleNewTabs.length).toBeGreaterThan(0) and expect(closeButton).not.toBeNull()), and if those assertions pass, call userEvent.click(closeButton) and retain the waitFor using canvas.queryAllByRole to verify the tab count decreased; this ensures the close operation is always executed and the test fails loudly when prerequisites are not met.
327-335: Silent catch may hide legitimate test failures.The
.catch(() => {})at lines 333-335 swallows all errors, including genuine failures that aren't timeout-related. Consider either removing the catch (let the loop proceed regardless) or logging for debugging purposes.♻️ Suggested approach
await waitFor( () => { const tabs = canvas.queryAllByRole('tab', { name: 'New tab' }); expect(tabs.length).toBeGreaterThanOrEqual(attempts); }, { timeout: 1000 } - ).catch(() => { - // Ignore timeout, we'll check overflow anyway - }); + ).catch(() => { + // Tab count check timed out; continue to overflow check + console.debug(`Tab count check timed out after ${attempts} tabs`); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/HelpPanel/HelpPanelContent.stories.tsx` around lines 327 - 335, The silent .catch on the waitFor call hides real failures; update the waitFor(...) usage in HelpPanelContent.stories (the call that uses canvas.queryAllByRole('tab', { name: 'New tab' }) and expects tabs.length) to stop swallowing errors—either remove the .catch so test failures surface, or catch only a timeout-specific error and log it (include context like the expected attempts and current length) before continuing; ensure you modify the catch handling around the waitFor invocation rather than the assertion itself.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/HelpPanel/HelpPanelContent.stories.tsx`:
- Around line 312-316: The story queries addTabButton via document.querySelector
but doesn't assert it exists before use; add an explicit precondition check
(e.g., throw or use expect) immediately after obtaining addTabButton to ensure
the element is non-null before entering the loop in this story (mirroring the
OpenAndCloseTabs approach), referencing the addTabButton variable so failures
show a clear message if the selector changes.
---
Nitpick comments:
In `@src/components/HelpPanel/HelpPanelContent.stories.tsx`:
- Around line 349-368: The test currently skips exercising the close path when
visibleNewTabs is empty or closeButton is null; update the block around
visibleNewTabs/firstTabItem/closeButton so it asserts preconditions before
proceeding (e.g., expect(visibleNewTabs.length).toBeGreaterThan(0) and
expect(closeButton).not.toBeNull()), and if those assertions pass, call
userEvent.click(closeButton) and retain the waitFor using canvas.queryAllByRole
to verify the tab count decreased; this ensures the close operation is always
executed and the test fails loudly when prerequisites are not met.
- Around line 327-335: The silent .catch on the waitFor call hides real
failures; update the waitFor(...) usage in HelpPanelContent.stories (the call
that uses canvas.queryAllByRole('tab', { name: 'New tab' }) and expects
tabs.length) to stop swallowing errors—either remove the .catch so test failures
surface, or catch only a timeout-specific error and log it (include context like
the expected attempts and current length) before continuing; ensure you modify
the catch handling around the waitFor invocation rather than the assertion
itself.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30c5d1f8-ec6d-4cf1-9b0a-69729614c8e7
📒 Files selected for processing (1)
src/components/HelpPanel/HelpPanelContent.stories.tsx
InsaneZein
approved these changes
Mar 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For RHCLOUD-45836)
Since we're running out of subtab room with the search + knowledgebase tabs, we're moving the status link to be beside the "Help" text. This PR also removes the "Chat with an Assistant" button as we've added the chatbot tab. I've also added storybook stories for the link + tab functionality as this wasn't covered anywhere.