Open virtual assistant on NotFound #3124
Merged
+306
−37
Conversation
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
justinorringer
requested review from
Hyperkid123,
ryelo,
karelhala and
florkbr
as code owners
Reviewer's GuideThis PR wires Jotai atoms to manage the Virtual Assistant’s open state and initial input, extends its route configuration to catch-all/404 routes, introduces a useVirtualAssistant hook and error boundary, enhances 404 and empty search pages with VA launch controls, and updates Cypress tests to mock and verify the new assistant behavior. Entity relationship diagram for new Virtual Assistant atoms in chromeStoreerDiagram
chromeStore ||--o{ virtualAssistantShowAssistantAtom : contains
chromeStore ||--o{ virtualAssistantOpenAtom : contains
chromeStore ||--o{ virtualAssistantStartInputAtom : contains
virtualAssistantShowAssistantAtom {
boolean value
}
virtualAssistantOpenAtom {
boolean value
}
virtualAssistantStartInputAtom {
string or undefined value
}
Class diagram for new and updated Virtual Assistant state managementclassDiagram
class useVirtualAssistant {
+openVA(startInput: string)
}
class virtualAssistantOpenAtom {
<<atom<boolean>>
}
class virtualAssistantShowAssistantAtom {
<<atom<boolean>>
}
class virtualAssistantStartInputAtom {
<<atom<string | undefined>>
}
useVirtualAssistant --> virtualAssistantOpenAtom : uses
useVirtualAssistant --> virtualAssistantShowAssistantAtom : uses
useVirtualAssistant --> virtualAssistantStartInputAtom : uses
Class diagram for updated VirtualAssistant route componentclassDiagram
class VirtualAssistant {
+isOpen: boolean
+setOpen(open: boolean): void
+startInput: string
+setStartInput(message: string): void
+showAssistant: boolean
+setShowAssistant(show: boolean): void
}
class ScalprumComponent {
+scope: string
+module: string
+showAssistant: boolean
+isOpen: boolean
+setOpen(open: boolean): void
+startInput: string
+setStartInput(message: string): void
}
VirtualAssistant --> ScalprumComponent : passes props
Class diagram for SilentErrorBoundary componentclassDiagram
class SilentErrorBoundary {
-hasError: boolean
-error: any
-errorInfo: any
+render(): ReactNode
+componentDidCatch(error, errorInfo): void
+static getDerivedStateFromError(): object
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @justinorringer - I've reviewed your changes - here's some feedback:
- The element in NotFoundRoute is used as a button without an href—consider swapping to a PatternFly or a native with proper ARIA attributes for accessibility.
- Catching 404s by adding '*' to
viableRoutesmay inadvertently match every route—consider usinguseLocationor a dedicated catch-all instead of maintaining a static list. - You recreate the
virtualAssistantPropsobject on every render, which may remount the Scalprum component; wrap it inuseMemoto avoid unnecessary re-initialization.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The <a> element in NotFoundRoute is used as a button without an href—consider swapping to a PatternFly <Button> or a native <button> with proper ARIA attributes for accessibility.
- Catching 404s by adding '*' to `viableRoutes` may inadvertently match every route—consider using `useLocation` or a dedicated catch-all <Route> instead of maintaining a static list.
- You recreate the `virtualAssistantProps` object on every render, which may remount the Scalprum component; wrap it in `useMemo` to avoid unnecessary re-initialization.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
justinorringer
commented
Jun 16, 2025
| const viableRoutes = ['/', '/insights/*', '/settings/*', '/subscriptions/overview/*', '/subscriptions/inventory/*', '/subscriptions/usage/*']; | ||
| const [isOpen, setOpen] = useAtom(virtualAssistantOpenAtom); | ||
| const [startInput, setStartInput] = useAtom(virtualAssistantStartInputAtom); | ||
| // TODO: If a route that results in a 404 is not in this list, the Virtual Assistant will not be displayed. :( |
There was a problem hiding this comment.
I'm not sure about the best way to get around the viableRoutes on 404 (I added the wildcard for testing)
Add the isHidden var to jotai and check it here? https://github.com/RedHatInsights/insights-chrome/blob/master/src/components/ChromeRoute/ChromeRoute.tsx#L35
There was a problem hiding this comment.
Met with Justin offline. Due to the nature of the VA (and it being a federated module) - we can simply render a scalprum component for VA on the 404 page (and in other locations) as the URL will not be predictable.
There is one issue we will need to solve however around state management (so on pages where the VA is already present and open we do not reset the chat history/reopen it). We can manage that state inside the VA itself like we have done for the notifications panel singleton instance.
|
@justinorringer we may want to mark this a draft so it's not accidentally merged (wildcard in viable routes shouldn't be merged). Also, I see some lint failures reported by the github action: Can we also include the JIRA in the description? |
|
/retest |
|
@florkbr @Hyperkid123 mind taking a look at the error boundary solution to fix the tests (not ideal) |
JetyAdam
reviewed
Jun 20, 2025
| @@ -0,0 +1,5 @@ | |||
| import { atom } from 'jotai'; | |||
|
|
|||
| export const virtualAssistantShowAssistantAtom = atom(false); | |||
There was a problem hiding this comment.
I believe that it's a good practice to define types for atoms. Even though TS will implicitly define its type by itself, it wouldn't catch errors if the value was accidentally changed
|
@justinorringer the 404 page looks wierd now: No relevant errors in console. we should only see the not found component, not be "error" component. |
There was a problem hiding this comment.
Hey @justinorringer - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/components/Search/EmptySearchState.tsx:36` </location>
<code_context>
+ {isOpenConfig ? (
+ <Content component="p" className="pf-v6-u-text-color-subtle">
+ Try searching Hybrid Cloud help or start a conversation with our{' '}
+ <a
+ onClick={() => {
+ openVA('');
+ }}
+ >
+ Virtual Assistant.
+ </a>
</code_context>
<issue_to_address>
Using an <a> tag without href for a button-like action may affect accessibility.
Since the <a> tag is used for an action, either use a <button> element or add role="button" and keyboard event handlers to ensure accessibility.
</issue_to_address>
### Comment 2
<location> `src/components/NotFoundRoute/NotFoundRoute.tsx:23` </location>
<code_context>
+
+ return (
+ <>
+ {isOpenConfig ? (
+ <EmptyState id="not-found">
+ <EmptyStateBody>
</code_context>
<issue_to_address>
Consider extracting the conditional button and rendering a single <EmptyState> to avoid duplicating markup.
```jsx
// Extract the conditional button into a small variable (or helper component)
// and render a single <EmptyState> tree:
const NotFoundRoute = () => {
const setShowAssistant = useSetAtom(virtualAssistantShowAssistantAtom);
const { openVA } = useVirtualAssistant();
const isOpenConfig = useFlag('platform.virtual-assistant.is-open-config');
useEffect(() => {
setShowAssistant(true);
}, [setShowAssistant]);
// only render the Button if isOpenConfig is true
const actionButton = isOpenConfig && (
<Button
onClick={() => openVA(`Contact my org admin.`)}
className="pf-v6-c-button pf-m-link"
>
Contact your org admin with the Virtual Assistant.
</Button>
);
return (
<EmptyState id="not-found">
<EmptyStateBody>
<InvalidObject />
{actionButton}
</EmptyStateBody>
</EmptyState>
);
};
```
This removes the duplicated `<EmptyState>` branches while preserving all behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
florkbr
reviewed
Jul 16, 2025
| @@ -69,6 +81,9 @@ const Wrapper = ({ | |||
| const [scalprumConfig, setScalprumConfig] = useAtom(scalprumConfigAtom); | |||
| const setModuleRoutes = useSetAtom(moduleRoutesAtom); | |||
| useEffect(() => { | |||
| console.log('Setting scalprum config and module routes in Wrapper'); | |||
There was a problem hiding this comment.
@justinorringer can we remove these console log statements?
florkbr
reviewed
Jul 16, 2025
| @@ -31,6 +33,27 @@ const defaultUser: ChromeUser = { | |||
| }, | |||
| }; | |||
|
|
|||
| const chromeAuthContextValue: ChromeAuthContextValue = { | |||
| ssoUrl: '', | |||
| doOffline: () => Promise.resolve(), | |||
There was a problem hiding this comment.
It might be nice to move this chromeAuthContextValue to a test-helper utility somewhere in the future in case other tests need this shape going forward. Not blocking.
florkbr
reviewed
Jul 16, 2025
| @@ -47,6 +47,11 @@ const initialScalprumConfig = { | |||
| appId: 'TestApp', | |||
| manifestLocation: '/foo/bar.json', | |||
| }, | |||
| virtualAssistant: { | |||
| name: 'virtualAssistant', | |||
There was a problem hiding this comment.
@justinorringer since we had to touch so many of these specs that setup the scalprum context - it may be nice to move this to a shared setup as well (in case we need to remove it in the future, or add more site wide modules). Not blocking
| ], | ||
| }); | ||
| }); | ||
|
|
||
| it('should render not found route if permissions are not met', () => { |
There was a problem hiding this comment.
Just to make sure I understand the test scenario. Would this cover something like a non-admin trying to view /iam/users which is restricted to org admins or users with appropriate assigned permissions?
florkbr
approved these changes
Jul 16, 2025
|
Konflux EC failure: Looking into it... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
https://issues.redhat.com/browse/RHCLOUD-32515
Summary by Sourcery
Enable the Virtual Assistant to open on any route (including 404s) by wiring Jotai-powered state for its visibility and initial input, updating its route configuration, and adding a prompt link on the NotFound page.
New Features:
Enhancements:
Chores:
Summary by Sourcery
Enable the Virtual Assistant to open on any route (including 404s) by introducing global Jotai state, expanding route configuration, and adding launch prompts on the NotFound and empty search pages.
New Features:
Enhancements:
Tests:
Chores: